'Re: <AWT Dev> RFR: [OpenJDK 2D-Dev] JDK-8215296 do not disable c99 on Solaris'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-awt-dev
Subject:    Re: <AWT Dev> RFR: [OpenJDK 2D-Dev] JDK-8215296 do not disable c99 on Solaris
From:       Magnus Ihse Bursie <magnus.ihse.bursie () oracle ! com>
Date:       2018-12-20 10:48:28
Message-ID: C332A2F0-D2EE-4AC1-B525-4DA974FB2D29 () oracle ! com
[Download RAW message or body]

Yes, you can add me as reviewer. 

/Magnus

19 dec. 2018 kl. 16:31 skrev Baesken, Matthias <matthias.baesken@sap.com>:

> > 
> > 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.bursie@oracle.com>
> > Sent: Montag, 17. Dezember 2018 16:11
> > To: Baesken, Matthias <matthias.baesken@sap.com>
> > Cc: 2d-dev@openjdk.java.net; erik.joelsson@oracle.com; build-
> > dev@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.baesken@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@oracle.com>
> > > > To: Erik Joelsson <erik.joelsson@oracle.com>, build-dev
> > > > <build-dev@openjdk.java.net>, "awt-dev@openjdk.java.net"
> > > > <awt-dev@openjdk.java.net>, 2d-dev <2d-dev@openjdk.java.net>
> > > > Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8215296 do not disable c99 on
> > > > Solaris
> > > > Message-ID: <5874d10e-db2d-8681-a54b-a1eeb6e45994@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@oracle.com
> > > > > <mailto:erik.joelsson@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"
> 


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic