RFR: [OpenJDK 2D-Dev] JDK-8215296 do not disable c99 on Solaris

David Holmes david.holmes at oracle.com
Wed Dec 19 21:47:53 UTC 2018 

On 20/12/2018 1:31 am, Baesken, Matthias wrote:
>>
>> Sounds like a simpler change, at least for now. Does it pass jdk-submit?
>>
> 
> Hello Magnus ,   pushed it to  jdk/submit  , however I do not expect  much info from it because David  told me Solaris is not tested there

I was incorrect.

>   (and the other platforms are not affected by my path).
> 
> However David tested it Oracle-internally   with success .

Yes.

> Can I add you and David as reviewers ?

Yes from me.

Thanks,
David

> 
> Best regards, Matthias
> 
> 
> 
>> -----Original Message-----
>> From: Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com>
>> Sent: Montag, 17. Dezember 2018 16:11
>> To: Baesken, Matthias <matthias.baesken at sap.com>
>> Cc: 2d-dev at openjdk.java.net; erik.joelsson at oracle.com; build-
>> dev at openjdk.java.net; awt-dev at openjdk.java.net
>> Subject: Re: RFR: [OpenJDK 2D-Dev] JDK-8215296 do not disable c99 on
>> Solaris
>>
>> Sounds like a simpler change, at least for now. Does it pass jdk-submit? Do
>> you intend to push to 12 or 13?
>>
>> Looks good to me, as long as it doesn't break anything.
>>
>> /Magnus
>>
>>> 17 dec. 2018 kl. 14:12 skrev Baesken, Matthias
>> <matthias.baesken at sap.com>:
>>>
>>>
>>> Hello,  please review
>>>
>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8215296.0/
>>>
>>> in my change just -xc99=%none  is removed, so we do not forbid c99
>> coding.
>>>
>>> The -Xa compile flag is kept,  no special additional settings are needed to
>> compile png/awt .
>>>
>>>
>>> Thanks, Matthias
>>>
>>>
>>>> ----------------------------------------------------------------------
>>>>
>>>> Message: 1
>>>> Date: Fri, 14 Dec 2018 15:39:26 +0100
>>>> From: Magnus Ihse Bursie <magnus.ihse.bursie at oracle.com>
>>>> To: Erik Joelsson <erik.joelsson at oracle.com>, build-dev
>>>>     <build-dev at openjdk.java.net>, "awt-dev at openjdk.java.net"
>>>>     <awt-dev at openjdk.java.net>, 2d-dev <2d-dev at openjdk.java.net>
>>>> Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8215296 do not disable c99 on
>>>>     Solaris
>>>> Message-ID: <5874d10e-db2d-8681-a54b-a1eeb6e45994 at oracle.com>
>>>> Content-Type: text/plain; charset=utf-8; format=flowed
>>>>
>>>>
>>>>
>>>>> On 2018-12-14 12:49, Magnus Ihse Bursie wrote:
>>>>>
>>>>> 13 dec. 2018 kl. 19:07 skrev Erik Joelsson <erik.joelsson at oracle.com
>>>>> <mailto:erik.joelsson at oracle.com>>:
>>>>>
>>>>>>
>>>>>>> On 2018-12-13 02:11, Magnus Ihse Bursie wrote:
>>>>>>>
>>>>>>>> -D_XPG6
>>>>>>>>
>>>>>>>> ??
>>>>>>> To be honest, I'm not completely sure about this. Without this
>>>>>>> define, the build failed with the following error message:
>>>>>>> Compiler or options invalid for pre-UNIX 03 X/Open applications and
>>>>>>> pre-2001 POSIX applications
>>>>>>>
>>>>>>> This was triggered by the following section in
>>>>>>> /usr/include/sys/feature_tests.h:
>>>>>>> /*
>>>>>>> * It is invalid to compile an XPG3, XPG4, XPG4v2, or XPG5 application
>>>>>>> * using c99.  The same is true for POSIX.1-1990, POSIX.2-1992,
>>>>>>> POSIX.1b,
>>>>>>> * and POSIX.1c applications. Likewise, it is invalid to compile an
>>>>>>> XPG6
>>>>>>> * or a POSIX.1-2001 application with anything other than a c99 or
>>>>>>> later
>>>>>>> * compiler.  Therefore, we force an error in both cases.
>>>>>>> */
>>>>>>> #if defined(_STDC_C99) && (defined(__XOPEN_OR_POSIX) &&
>>>>>>> !defined(_XPG6))
>>>>>>> #error "Compiler or options invalid for pre-UNIX 03 X/Open
>>>>>>> applications \
>>>>>>>         and pre-2001 POSIX applications"
>>>>>>> #elif !defined(_STDC_C99) && \
>>>>>>>         (defined(__XOPEN_OR_POSIX) && defined(_XPG6))
>>>>>>> #error "Compiler or options invalid; UNIX 03 and POSIX.1-2001
>>>>>>> applications \
>>>>>>>         require the use of c99"
>>>>>>> #endif
>>>>>>>
>>>>>>> The solution, as also hinted to by searching for other resolutions
>>>>>>> to this error online, was to provide the _XPG6 system define. But
>>>>>>> exactly how we end up in feature_tests.h with __XOPEN_OR_POSIX
>> set,
>>>>>>> without _XPG6 set, and only when compiling this library and not
>>>>>>> others, I don't know. I also don't understand what the XPG standard
>>>>>>> refers to, nor what versions 2-5 means or what version 6 has that
>>>>>>> differs from them.
>>>>>>>
>>>>>>> By setting this flag, I am telling solaris include headers that we
>>>>>>> want to compile using the XPG standard version 6, instead of an
>>>>>>> older one. It solves the problem. I am happy enough with this. Are
>> you?
>>>>>> It looks like this comes from libpng. It has this in
>>>>>> src/java.desktop//share/native/libsplashscreen/libpng/pngpriv.h:
>>>>>>
>>>>>> /* Feature Test Macros.  The following are defined here to ensure
>>>>>> that correctly
>>>>>> * implemented libraries reveal the APIs libpng needs to build and
>>>>>> hide those
>>>>>> * that are not needed and potentially damaging to the compilation.
>>>>>> *
>>>>>> * Feature Test Macros must be defined before any system header is
>>>>>> included (see
>>>>>> * POSIX 1003.1 2.8.2 "POSIX Symbols."
>>>>>> *
>>>>>> * These macros only have an effect if the operating system supports
>>>>>> either
>>>>>> * POSIX 1003.1 or C99, or both.  On other operating systems
>>>>>> (particularly
>>>>>> * Windows/Visual Studio) there is no effect; the OS specific tests
>>>>>> below are
>>>>>> * still required (as of 2011-05-02.)
>>>>>> */
>>>>>> #ifndef _POSIX_SOURCE
>>>>>> # define _POSIX_SOURCE 1 /* Just the POSIX 1003.1 and C89 APIs */
>>>>>> #endif
>>>>>>
>>>>>> This in turn triggers _XOPEN_OR_POSIX to be defined in
>>>>>> /usr/include/sys/feature_tests.h and so triggers the error.
>>>>>>
>>>>>> What I'm not clear about is if libpng is trying to declare that it
>>>>>> should not be compiled with any newer standards, and so by doing
>>>>>> that, we risk introducing problems. Reading in the system header, it
>>>>>> seems the _XPG6 macro is internal and should not be used by the
>>>>>> application. It's derived from _XOPEN_SOURCE=600 or
>>>>>> _POSIX_C_SOURCE=200112L which is what applications should use.
>>>>>
>>>>> Interesting. We should probably define one, or both of these. Perhaps
>>>>> globally for all native files and compilers. It might have been the
>>>>> case that the solstudio compiler set _POSIX_C_SOURCE for us before,
>>>>> prior to setting -std=c99. The following stack overflow article claims
>>>>> that this is at least the behavior of gcc/clang:
>>>>>
>>>>> https://stackoverflow.com/questions/21867897/c89-and-posix-at-the-
>>>> same-time
>>>>>
>>>>>
>>>>> So we might have had an implicit _POSIX_C_SOURCE that we now miss.
>>>>> That would explain why this starts to fail. I'll see if I can confirm
>>>>> this the next time I log into a Solaris computer.
>>>> Of course it was not as simple. Setting:
>>>>    ifeq ($(OPENJDK_TARGET_OS), solaris)
>>>>      LIBSPLASHSCREEN_CFLAGS += -D_POSIX_C_SOURCE=200112L -
>>>> D_XOPEN_SOURCE=600
>>>>    endif
>>>>
>>>> instead made us fail with:
>>>> open/src/java.desktop/unix/native/libsplashscreen/splashscreen_sys.c",
>>>> line 143: error: incomplete struct/union/enum timezone: tz
>>>>
>>>> I don't have more time to dig into this now. Overall, changes such as
>>>> these make it all feel a bit scary; I recommend that any change to this
>>>> be made in JDK 13 and not 12.
>>>>
>>>> /Magnus
>>>>>
>>>>> Otoh, the same article claims, and it sounds reasonable, that we
>>>>> should set these variables ourself, to be well behaved and to minimize
>>>>> surprises. And I think this applies to all unix platforms, regardless
>>>>> of compiler being used. I'll see if I can kick off a test job with
>>>>> this to see how/if it influences other platforms. But it sounds like
>>>>> something we should do; the level of posix conformance should be
>>>>> controlled by us, not left to chance. We also need to verify, of
>>>>> course, that all platforms we want to support is capable of
>>>>> supporting  _POSIX_C_SOURCE=200112L. I doubt there's a problem
>>>> though.
>>>>> Possibly on AIX...
>>>>>
>>>>> /Magnus
>>>>>
>>>>>>
>>>>>> So the the question is, is it ok to override the requirements of
>>>>>> libpng or should it receive special treatment? If we are fine with
>>>>>> overriding, then we should use one of the public APIs instead.
>>>>>>
>>>>>> /Erik
>>>>>>
>>>>>>> /Magnus
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>>> On 13/12/2018 7:02 am, Magnus Ihse Bursie wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On 2018-12-12 20:08, Magnus Ihse Bursie wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> On 2018-12-12 19:12, Magnus Ihse Bursie wrote:
>>>>>>>>>>>  From the bug report:
>>>>>>>>>>> "Currently  we disable C99 in the Solaris build by setting
>>>>>>>>>>> -xc99=%none%.
>>>>>>>>>>> This differs from a lot of other build environments like
>>>>>>>>>>> gcc/Linux or VS2013/2017 on Windows where C99 features work.
>>>>>>>>>>> We should remove this difference on Solaris and remove or
>>>>>>>>>>> replace the setting.
>>>>>>>>>>>
>>>>>>>>>>> Kim Barrett mentioned :
>>>>>>>>>>> "I merely mentioned the C++14 work as evidence that removing
>>>>>>>>>>> -xc99=%none% didn?t appear harmful."
>>>>>>>>>>> However it will take more time until  the C++14 change is in."
>>>>>>>>>>>
>>>>>>>>>>> I am currently running a test build on our CI build system to
>>>>>>>>>>> confirm that this does not break the Solaris build (but I'd be
>>>>>>>>>>> highly surprised if it did). I will not push this until the
>>>>>>>>>>> builds are cleared.
>>>>>>>>>> Of course it was not that simple... :-( Two AWT libraries (at
>>>>>>>>>> least) failed to build. I'm currently investigating if there's a
>>>>>>>>>> simple fix to that.
>>>>>>>>> New attempt, that fixes the two AWT libraries:
>>>>>>>>> WebRev:
>>>>>>>>> http://cr.openjdk.java.net/~ihse/JDK-8215296-build-solstudio-
>> with-
>>>> c99/webrev.01
>>>>>>>>> <http://cr.openjdk.java.net/%7Eihse/JDK-8215296-build-solstudio-
>>>> with-c99/webrev.01>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Now this passes the CI build test.
>>>>>>>>>
>>>>>>>>> /Magnus
>>>>>>>>>>
>>>>>>>>>> /Magnus
>>>>>>>>>>>
>>>>>>>>>>> /Magnus
>>>>>>>>>>>
>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8215296
>>>>>>>>>>> Patch inline:
>>>>>>>>>>> diff --git a/make/autoconf/flags-cflags.m4
>>>>>>>>>>> b/make/autoconf/flags-cflags.m4
>>>>>>>>>>> --- a/make/autoconf/flags-cflags.m4
>>>>>>>>>>> +++ b/make/autoconf/flags-cflags.m4
>>>>>>>>>>> @@ -559,7 +559,7 @@
>>>>>>>>>>> TOOLCHAIN_CFLAGS="-errshort=tags"
>>>>>>>>>>>
>>>>>>>>>>> TOOLCHAIN_CFLAGS_JDK="-mt $TOOLCHAIN_FLAGS"
>>>>>>>>>>> - TOOLCHAIN_CFLAGS_JDK_CONLY="-xc99=%none -xCC -Xa -
>> W0,-
>>>> noglobal
>>>>>>>>>>> $TOOLCHAIN_CFLAGS" # C only
>>>>>>>>>>> + TOOLCHAIN_CFLAGS_JDK_CONLY="-std=c99 -xCC -W0,-
>> noglobal
>>>>>>>>>>> $TOOLCHAIN_CFLAGS" # C only
>>>>>>>>>>> TOOLCHAIN_CFLAGS_JDK_CXXONLY="-features=no%except -
>>>> norunpath
>>>>>>>>>>> -xnolib" # CXX only
>>>>>>>>>>> TOOLCHAIN_CFLAGS_JVM="-template=no%extdef -
>>>> features=no%split_init \
>>>>>>>>>>>          -library=stlport4 -mt -features=no%except
>>>>>>>>>>> $TOOLCHAIN_FLAGS"
>>>
>

More information about the build-dev mailing list