RFR: build pragma error with gcc 4.4.7

Sat Apr 21 15:18:48 UTC 2018

On 19 March 2018 at 23:23, Kim Barrett <kim.barrett at oracle.com> wrote:
>> On Mar 16, 2018, at 6:48 AM, Michal Vala <mvala at redhat.com> wrote:
>>
>> Hi,
>>
>> I've been trying to build latest jdk with gcc 4.4.7 and I hit compile error due to pragma used in function:
>>
>> /mnt/ramdisk/openjdk/src/hotspot/os/linux/os_linux.inline.hpp:103: error: #pragma GCC diagnostic not allowed inside functions
>>
>>
>> I'm sending little patch that fixes the issue by wrapping whole function. I've also created a macro for ignoring deprecated declaration inside compilerWarnings.hpp to line up with others.
>>
>> Can someone please review? If it's ok, I would also need a sponsor.
>>
>>
>> diff -r 422615764e12 src/hotspot/os/linux/os_linux.inline.hpp
>> --- a/src/hotspot/os/linux/os_linux.inline.hpp        Thu Mar 15 14:54:10 2018 -0700
>> +++ b/src/hotspot/os/linux/os_linux.inline.hpp        Fri Mar 16 10:50:24 2018 +0100
>> @@ -96,13 +96,12 @@
>>   return ::ftruncate64(fd, length);
>> }
>>
>> -inline struct dirent* os::readdir(DIR* dirp, dirent *dbuf)
>> -{
>> // readdir_r has been deprecated since glibc 2.24.
>> // See https://sourceware.org/bugzilla/show_bug.cgi?id=19056 for more details.
>> -#pragma GCC diagnostic push
>> -#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
>> -
>> +PRAGMA_DIAG_PUSH
>> +PRAGMA_DEPRECATED_IGNORED
>> +inline struct dirent* os::readdir(DIR* dirp, dirent *dbuf)
>> +{
>>   dirent* p;
>>   int status;
>>   assert(dirp != NULL, "just checking");
>> @@ -114,11 +113,11 @@
>>   if((status = ::readdir_r(dirp, dbuf, &p)) != 0) {
>>     errno = status;
>>     return NULL;
>> -  } else
>> +  } else {
>>     return p;
>> -
>> -#pragma GCC diagnostic pop
>> +  }
>> }
>> +PRAGMA_DIAG_POP
>>
>> inline int os::closedir(DIR *dirp) {
>>   assert(dirp != NULL, "argument is NULL");
>> diff -r 422615764e12 src/hotspot/share/utilities/compilerWarnings.hpp
>> --- a/src/hotspot/share/utilities/compilerWarnings.hpp        Thu Mar 15 14:54:10 2018 -0700
>> +++ b/src/hotspot/share/utilities/compilerWarnings.hpp        Fri Mar 16 10:50:24 2018 +0100
>> @@ -48,6 +48,7 @@
>> #define PRAGMA_FORMAT_NONLITERAL_IGNORED _Pragma("GCC diagnostic ignored \"-Wformat-nonliteral\"") \
>>                                          _Pragma("GCC diagnostic ignored \"-Wformat-security\"")
>> #define PRAGMA_FORMAT_IGNORED _Pragma("GCC diagnostic ignored \"-Wformat\"")
>> +#define PRAGMA_DEPRECATED_IGNORED _Pragma("GCC diagnostic ignored \"-Wdeprecated-declarations\"")
>>
>> #if defined(__clang_major__) && \
>>       (__clang_major__ >= 4 || \
>>
>>
>> Thanks!
>>
>> --
>> Michal Vala
>> OpenJDK QE
>> Red Hat Czech
>
> Given that there seem to be no callers of os::readdir that share the
> DIR* among multiple threads, it would seem easier to just replace the
> use of ::readdir_r with ::readdir.  That seems to be the intent in the
> deprecation decision; use ::readdir, and either don't share a DIR*
> among threads, or use external locking when doing so.
>

That was my analysis when I looked at this as well. Something as simple as:

diff --git a/src/os/linux/vm/os_linux.inline.hpp
b/src/os/linux/vm/os_linux.inline.hpp
--- a/src/os/linux/vm/os_linux.inline.hpp
+++ b/src/os/linux/vm/os_linux.inline.hpp
@@ -116,19 +116,9 @@

 inline struct dirent* os::readdir(DIR* dirp, dirent *dbuf)
 {
-  dirent* p;
-  int status;
   assert(dirp != NULL, "just checking");

-  // NOTE: Linux readdir_r (on RH 6.2 and 7.2 at least) is NOT like the POSIX
-  // version. Here is the doc for this function:
-  // http://www.gnu.org/manual/glibc-2.2.3/html_node/libc_262.html
-
-  if((status = ::readdir_r(dirp, dbuf, &p)) != 0) {
-    errno = status;
-    return NULL;
-  } else
-    return p;
+  return ::readdir(dirp);
 }

 inline int os::closedir(DIR *dirp) {

has been working fine for me locally for a while. I believe Michal is going to
post an updated version of that, along with some cleanup of dbuf usage
in Linux-only code (as dbuf is now unused).

However, I wouldn't suggest backporting such a change to older JDKs.
There, we should backport the changeset to mute it, updating it to
work with older compilers. I already did so in OpenJDK 7.

> There are also problems with the patch as provided.
>
> (1) Since PRAGMA_DIAG_PUSH/POP do nothing in the version of gcc this
> change is being made in support of, the warning would be disabled for
> all following code in any translation unit that includes this file.
> That doesn't seem good.

No, but it's really the only solution on those compilers. We have such
usage already elsewhere e.g.

// Silence -Wformat-security warning for fatal()
PRAGMA_DIAG_PUSH
PRAGMA_FORMAT_NONLITERAL_IGNORED
  fatal(buf);
PRAGMA_DIAG_POP
  return true; // silence compiler warnings
}
in src/hotspot/os_cpu/linux_zero/os_linux_zero.cpp

If there are other warnings, then they will picked up on newer compilers,
especially when building with -Werror. I don't think it's likely people are
doing development on older compilers, but rather that we have to use
them to build for older platforms.

This isn't the first time I've encountered something which built fine locally,
and then failed when building on an old version of RHEL, and I suspect
it won't be the last.

>
> (2) The default empty definition for PRAGMA_DEPRECATED_IGNORED is
> missing.  That means the macro can't be used in shared code, in which
> case having defined in (shared) compilerWarnings.hpp is questionable.
>

Yes, I don't see the need to change that line (other than for consistency)
and didn't when I made the same change in 7:
http://hg.openjdk.java.net/jdk7u/jdk7u/hotspot/rev/2a2721def4a0#l7.2

-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Web Site: http://fuseyism.com
Twitter: https://twitter.com/gnu_andrew_java
PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222

