> > 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 (and the other platforms are not affected by my path). However David tested it Oracle-internally with success . Can I add you and David as reviewers ? Best regards, Matthias > -----Original Message----- > From: Magnus Ihse Bursie <magnus.ihse.bur...@oracle.com> > Sent: Montag, 17. Dezember 2018 16:11 > To: Baesken, Matthias <matthias.baes...@sap.com> > Cc: 2d-...@openjdk.java.net; erik.joels...@oracle.com; build- > d...@openjdk.java.net; awt-...@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.baes...@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.bur...@oracle.com> > >> To: Erik Joelsson <erik.joels...@oracle.com>, build-dev > >> <build-dev@openjdk.java.net>, "awt-...@openjdk.java.net" > >> <awt-...@openjdk.java.net>, 2d-dev <2d-...@openjdk.java.net> > >> Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8215296 do not disable c99 on > >> Solaris > >> Message-ID: <5874d10e-db2d-8681-a54b-a1eeb6e45...@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.joels...@oracle.com > >>> <mailto:erik.joels...@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" > >
