17 dec. 2018 kl. 16:24 skrev Baesken, Matthias <matthias.baes...@sap.com>:
>> Sounds like a simpler change, at least for now. Does it pass jdk-submit? Do >> you intend to push to 12 or 13? > > Hi Magus, thanks for the review. > I think it is safer to go for 13 , what's your opinion ? Agree. /Magnus > > I put it into our internal build+test queue first , after this is fine , I > will go for jdk-submit (hopefully it works ). > > 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-dev@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-...@openjdk.java.net>, "awt-dev@openjdk.java.net" >>>> <awt-dev@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" >>> >