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

Tue Jan 15 11:53:09 UTC 2019

Hi Patrick,

On 15/01/2019 7:24 pm, Patrick Zhang wrote:
> In case the attachment may get dropped by mailing system, I paste the udiffs in text here.

Yes attachments are generally dropped.

> ./src/jdk.pack/share/native/common-unpack/zip.cpp
> 
> rev 53300<https://bugs.openjdk.java.net/browse/JDK-53300> : 8215976<https://bugs.openjdk.java.net/browse/JDK-8215976>: Fix gmtime_r declaration conflicts in zip.cpp with linux header files
> 
> ________________________________
> 
> @@ -414,13 +414,13 @@
> 
>     return y < 1980 ? dostime(1980, 1, 1, 0, 0, 0) :
> 
>       (((uLong)y - 1980) << 25) | ((uLong)n << 21) | ((uLong)d << 16) |
> 
>       ((uLong)h << 11) | ((uLong)m << 5) | ((uLong)s >> 1);
> 
> }
> 
> -#ifdef _REENTRANT // solaris
> 
> -extern "C" struct tm *gmtime_r(const time_t *, struct tm *);
> 
> -#else
> 
> +// Linux, Solaris, OS X, etc have gmtime_r for _REENTRANT,

There is no need to mention _REENTRANT here - it is not relevant. I 
suggest simply:

// Non-windows platforms need gmtime_r for thread-safety,

Thanks,
David

> +// while gmtime for Windows is already thread-safe
> 
> +#ifdef _MSC_VER // Windows
> 
> #define gmtime_r(t, s) gmtime(t)
> 
> #endif
> 
> /*
> 
>    * Return the Unix time in DOS format
> 
>    */
> 
> 
> 
> 
> 
> Regards
> 
> Patrick
> 
> 
> 
> -----Original Message-----
> From: core-libs-dev <core-libs-dev-bounces at openjdk.java.net> On Behalf Of Patrick Zhang
> Sent: Tuesday, January 15, 2019 4:57 PM
> To: Roger Riggs <Roger.Riggs at oracle.com>
> Cc: core-libs-dev <core-libs-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
> 
> 
> 
> Hi Roger,
> 
> 
> 
> Thanks for your review firstly.
> 
> Attached is the latest patch rebased on today's tip of jdk/jdk (http://hg.openjdk.java.net/jdk/jdk/rev/0b2574a2a6c7).
> 
> <<JDK-8215976.webrev.01.zip>>
> 
> 
> 
> Yes there is a "#ifndef _MSC_VER" at line 36, while I think we'd better keep this "#ifdef _MSC_VER" here, as such the declaration of gmtime_r can be near to where it gets used (line 436 in the patch).
> 
> 
> 
> FYI. I sent out an almost same patch early on 3 Jan.
> 
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/057679.html
> 
> <<JDK-8215976.webrev.00.zip>>
> 
> 
> 
> Regards
> 
> Patrick
> 
> 
> 
> From: Roger Riggs <Roger.Riggs at oracle.com<mailto:Roger.Riggs at oracle.com>>
> 
> Sent: Monday, January 14, 2019 11:49 PM
> 
> To: Patrick Zhang <patrick at os.amperecomputing.com<mailto:patrick at os.amperecomputing.com>>
> 
> Cc: core-libs-dev <core-libs-dev at openjdk.java.net<mailto:core-libs-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
> 
> 
> 
> Hi Patrick,
> 
> 
> 
> Please re-post the entire proposed patch based on the JDK 13 repo.
> 
> 
> 
> BTW, there is already a "#ifndef _MSC_VER" at line 36.
> 
> 
> 
> Thanks, Roger
> 
> 
> 
> On 01/14/2019 09:02 AM, Magnus Ihse Bursie wrote:
> 
> On 2019-01-02 00:11, David Holmes wrote:
> 
> 
> 
> Hi Patrick,
> 
> 
> 
> On 13/12/2018 1:23 pm, Patrick Zhang wrote:
> 
> 
> 
> Ping...
> 
> 
> 
> Apply for a sponsor for this simple patch.
> 
> 
> 
> Seems no one wants to own this one :(
> 
> For what it's worth, it looks good to me. (But I'd like to see a core library developer chime in as well.)
> 
> 
> 
> /Magnus
> 
> 
> 
> 
> 
> 
> 
> 
> 
> I doubt if I could have permission to file the issue/bug report anywhere, could anyone kindly give a guidance? Thanks.
> 
> 
> 
> I have filed:
> 
> 
> 
> https://bugs.openjdk.java.net/browse/JDK-8215976
> 
> 
> 
> to cover this and linked to the discussion threads.
> 
> 
> 
> The appropriate owners still need to review this, but I'm not sure who that may be these days.
> 
> 
> 
> Cheers,
> 
> David
> 
> -----
> 
> 
> 
> 
> 
> 
> 
> Regards
> 
> Patrick
> 
> 
> 
> -----Original Message-----
> 
> From: core-libs-dev mailto:core-libs-dev-bounces at openjdk.java.net On Behalf Of Patrick Zhang
> 
> Sent: Thursday, December 6, 2018 4:28 PM
> 
> To: mailto:core-libs-dev at openjdk.java.net; David Holmes mailto:david.holmes at oracle.com
> 
> Cc: Florian Weimer mailto:fweimer at redhat.com
> 
> Subject: RE: OpenJDK fails to build with GCC when the #include<time.h> inside zip.cpp comes from a non-sysroot path
> 
> 
> 
> To All,
> 
> Who could help sponsor this simple patch (may require a wide range of building tests)? Thanks in advance.
> 
> 
> 
> Regards
> 
> Patrick
> 
> 
> 
> -----Original Message-----
> 
> From: David Holmes mailto:david.holmes at oracle.com
> 
> Sent: Monday, December 3, 2018 8:11 AM
> 
> To: Patrick Zhang mailto:patrick at os.amperecomputing.com; Florian Weimer mailto:fweimer at redhat.com
> 
> Cc: mailto:core-libs-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
> 
> 
> 
> Hi Patrick,
> 
> 
> 
> On 30/11/2018 11:41 pm, Patrick Zhang wrote:
> 
> 
> 
> Thanks Florian, the "-isystem /usr/include" is helpful to my case, I see gcc.gnu.org says that "it gets the same special treatment that is applied to the standard system directories". As such the issue gets hidden (error suppressed).
> 
> 
> 
> Hi David,
> 
> Thanks for your suggestion. My intention was to limit the influence range as far as I could since I don't have other systems except CentOS/Fedora to verify (even just smoke test) all paths.
> 
> 
> 
> You'd need some assistance testing a wider range of platforms but that can be arranged.
> 
> 
> 
> 
> 
> In addition, if we make below update, does it mean the macro " _REENTRANT " can be removed too? This is probably the only place where _REENTRANT gets used AFAIK.
> 
> _REENTRANT is also examined by system headers. It's probably legacy but would require more investigation.
> 
> 
> 
> Cheers,
> 
> David
> 
> 
> 
> 
> 
> #ifdef _MSC_VER // Windows
> 
> #define gmtime_r(t, s) gmtime(t)
> 
> #endif
> 
> 
> 
> Regards
> 
> Patrick
> 
> 
> 
> -----Original Message-----
> 
> From: Florian Weimer mailto:fweimer at redhat.com
> 
> Sent: Thursday, November 29, 2018 8:02 PM
> 
> To: David Holmes mailto:david.holmes at oracle.com
> 
> Cc: Patrick Zhang mailto:patrick at os.amperecomputing.com;
> 
> mailto:jdk-dev at openjdk.java.net; mailto:core-libs-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
> 
> 
> 
> * David Holmes:
> 
> 
> 
> 
> 
> This should really be being discussed on core-libs-dev.
> 
> 
> 
> Okay, moving the conversation.
> 
> 
> 
> 
> 
> 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
> 
> 
> 
> That looks much cleaner.
> 
> 
> 
> Thanks,
> 
> Florian
> 
> 
> 
> 