OpenJDK fails to build with GCC when the #include<time.h> inside zip.cpp comes from a non-sysroot path

[David Holmes]

Hi Patrick,

This should really be being discussed on core-libs-dev.

On 29/11/2018 1:17 pm, Patrick Zhang wrote:
> 
> Hi Florian
> 
> Thanks for your comments
> 
>>> I doubt it has anything to do with a sysroot vs non-sysroot configuration.
> 
> The tests are done on two systems respectively, CentOS 7.4 (glibc 2.17) and Fedora 28 (glibc 2.27).
> GCC 9 can be used as well to reproduce the issue as well like this:
> 1. /opt/gcc900/bin/g++ -c timetest.cpp
> 2. /opt/gcc900/bin/g++ -I<another-root>/usr/include -c timetest.cpp
> 3. /opt/gcc900/bin/g++ --sysroot=<another-root> -c timetest.cpp
> 4. /opt/gcc900/bin/g++ --sysroot=<another-root> -I/usr/include -c  timetest.cpp
> In above experiments, #1, #3 are good, because the time.h they found are under their sysroot path and treated as system header file (linemarker 3), while #2, #4 are bad since time.h comes from a non-sysroot path.
> 
>>> the comment in the OpenJDK is wrong and the declaration is included even on non-Solaris systems.
> 
> yes, the macro _REENTRANT is not specific for solaris system, instead it is for generic linux and gcc environment
> See jdk/make/autoconf/flags-cflags.m4
> if test "x$OPENJDK_TARGET_OS" = xlinux; then
>    CFLAGS_OS_DEF_JVM="-DLINUX"
>    CFLAGS_OS_DEF_JDK="-D_GNU_SOURCE -D_REENTRANT -D_LARGEFILE64_SOURCE"
> 
> if test "x$TOOLCHAIN_TYPE" = xgcc; then
>    ALWAYS_DEFINES_JVM="-D_GNU_SOURCE -D_REENTRANT"
> 
>>> so replicating the declaration from <time.h> is a losing battle.
> 
> Agree, as the declaration may be a redundant one in most occasions, what about changing it like below? I tested in my env it works well.
> 
> diff -r 70a423caee44 src/share/native/com/sun/java/util/jar/pack/zip.cpp
> --- a/src/share/native/com/sun/java/util/jar/pack/zip.cpp	Tue Oct 09 08:33:33 2018 +0100
> +++ b/src/share/native/com/sun/java/util/jar/pack/zip.cpp	Wed Nov 28 22:13:12 2018 -0500
> @@ -415,9 +415,7 @@
>       ((uLong)h << 11) | ((uLong)m << 5) | ((uLong)s >> 1);
>   }
>   
> -#ifdef _REENTRANT // solaris
> -extern "C" struct tm *gmtime_r(const time_t *, struct tm *);
> -#else
> +#if !defined(_REENTRANT) // linux
>   #define gmtime_r(t, s) gmtime(t)
>   #endif
>   /*

Under the theme "two wrongs don't make a right" the use of _REENTRANT 
here is totally inappropriate AFAICS. It seems to be a misguided attempt 
at determining whether we need the thread-safe gmtime_r or not - and the 
answer to that should always be yes IMHO.

We define _REENTRANT for:
- linux
- gcc
- xlc

So the original code will define:

extern "C" struct tm *gmtime_r(const time_t *, struct tm *);

for linux (and AIX with xlc?) but not Solaris, OS X or Windows.

But Solaris has gmtime_r anyway. So the existing code seems a really 
convoluted hack. AFAICS we have gmtime_r everywhere but Windows (where 
gmtime is already thread-safe). So it seems to me that all we should 
need here is:

-#ifdef _REENTRANT // solaris
-extern "C" struct tm *gmtime_r(const time_t *, struct tm *);
-#else
+#ifdef _MSC_VER // Windows
  #define gmtime_r(t, s) gmtime(t)
  #endif

Cheers,
David

> 
>>> Which system needs the gmtime_r macro definition?  Windows?
> 
> I think so, gmtime(..) is the common definition.
> 
> Thanks
> Patrick
> 
> From: Florian Weimer <fweimer at redhat.com>
> Sent: Wednesday, November 28, 2018 11:26 PM
> To: Patrick Zhang
> Cc: jdk-dev at openjdk.java.net
> Subject: Re: OpenJDK fails to build with GCC when the #include<time.h> inside zip.cpp comes from a non-sysroot path
>    
> 
> * Patrick Zhang:
> 
>> When I build on the latest jdk/jdk code base with GCC 8.2 (configured
>> sysroot as a customized path instead of default root directory of the
>> linux system),
> 
> I doubt it has anything to do with a sysroot vs non-sysroot
> configuration.
> 
>> it failed with error:
>> "jdk.pack/share/native/common-unpack/zip.cpp:420:23: error:
>> declaration of 'tm* gmtime_r(const time_t*, tm*)' has a different
>> exception specifier".
> 
> The declaration of gmtime_r should be removed.  Apparently, the comment
> in the OpenJDK is wrong and the declaration is included even on
> non-Solaris systems.
> 
> With glibc, you can declare your own gmtime_r function this way, but not
> if you include the <time.h> header.  GCC 9 will also warn if we add
> additional attributes (e.g. for nonnull warnings), so replicating the
> declaration from <time.h> is a losing battle.
> 
> Which system needs the gmtime_r macro definition?  Windows?
> 
> Thanks,
> Florian
>        
>