Hi, You are right, there is a better place for this,
in make/lib/Awt2dLibraries.gmk b/make/lib/Awt2dLibraries.gmk is the line LIBSPLASHSCREEN_CFLAGS += -DSPLASHSCREEN -DPNG_NO_MMX_CODE \ so, someone has already done this here for x86, so I just need to add the equivalent for aarch64 LIBSPLASHSCREEN_CFLAGS += -DSPLASHSCREEN -DPNG_NO_MMX_CODE -DPNG_ARM_NEON_OPT=0 \ Note: I have not conditionalised this on OPENJDK_TARGET_CPU_ARCH as the corresponding x86 code is not conditionalised and I really don't think the symbol PNG_ARM_NEON_OPT will have any effect on any other architectures. Erik, Magnus & David, I have listed you as reviewers in the following webrev. If you are happy with this please let me know and I will push. http://cr.openjdk.java.net/~enevill/8078245/webrev.02/ All the best, Ed. On Mon, 2015-05-04 at 17:03 +0200, Erik Joelsson wrote: > Right, that does make sense. This isn't a toolchain global flag like > some others we've had to deal with lately, but rather seems pretty > specific to the png code, or am I missing something? If that's the case, > then it's better added for the specific library. > > /Erik > > On 2015-05-04 16:43, Magnus Ihse Bursie wrote: > > Why not just for the library that needs it? > > > > /Magnus > > > >> 4 maj 2015 kl. 11:05 skrev Erik Joelsson <erik.joels...@oracle.com>: > >> > >> Hello, > >> > >> I think this looks good enough. The flags handling is still a big mess so > >> even if there were a better place for this, I couldn't say where. > >> > >> /Erik > >> > >>> On 2015-04-30 07:26, David Holmes wrote: > >>> Hi Nevill, > >>> > >>> Just realized this was sent to hotspot-dev (attempting bcc) but is not a > >>> hotspot issue. With your new approach this is a build issue so cc'ing > >>> build-dev. > >>> > >>> The new approach seems better to me but build folk need to confirm the > >>> placement. > >>> > >>> Thanks, > >>> David > >>> > >>>> On 29/04/2015 8:52 PM, Edward Nevill wrote: > >>>>> On Fri, 2015-04-24 at 17:11 +1000, David Holmes wrote: > >>>>> Hi Ed, > >>>>> > >>>>>> On 21/04/2015 7:16 PM, Edward Nevill wrote: > >>>>>> Hi, > >>>>>> > >>>>>> The current jdk9 tip fails to build from source on aarch64 with the > >>>>>> following error message > >>>>>> > >>>>>> /home/ed/build/1504/dev/build/linux-aarch64-normal-server-release/support/native/java.desktop/libsplashscreen/pngrutil.o: > >>>>>> In function `png_init_filter_functions': > >>>>>> /home/ed/build/1504/dev/jdk/src/java.desktop/share/native/libsplashscreen/libpng/pngrutil.c:3947: > >>>>>> undefined reference to `png_init_filter_functions_neon' > >>>>>> collect2: error: ld returned 1 exit status > >>>>>> > >>>>>> The following webrev gets it building again. > >>>>>> > >>>>>> http://cr.openjdk.java.net/~enevill/8078245/webrev.00/ > >>>>> Shouldn't the guard be Aarch64 specific rather than just __arm__ ? I'm > >>>>> also wondering how we would get __ARM_NEON defined but not __arm__? > >>>> On arm 32 bit gcc defines the symbol __ARM_NEON if the flag -mfpu=neon > >>>> is specified. Since this is not specified as part of the OpenJDK build > >>>> the symbol is not defined and the build succeeds. > >>>> > >>>> On aarch64 the symbol __ARM_NEON is always defined (the theory being > >>>> that aarch64 always supports Neon). Personally I think this is borken as > >>>> it causes builds to fail (and not just OpenJDK, several other projects > >>>> have had the same build failure - try googling the above error message). > >>>> But we are stuck with gcc as it is. > >>>> > >>>> However, I don't like the above fix because it not only modifies the > >>>> jdk, it modifies an external component which is pulled into jdk, which > >>>> means every time a new revision of linpng is pulled in, the patch will > >>>> have to be applied again. > >>>> > >>>> A better approach I think is to define the symbol PNG_ARM_NEON_OPT=0 in > >>>> the build (only if aarch64). > >>>> > >>>> The failing code in pngpriv.h reads > >>>> > >>>> #ifndef PNG_ARM_NEON_OPT > >>>> > >>>> ... > >>>> > >>>> # if (defined(__ARM_NEON__) || defined(__ARM_NEON)) && \ > >>>> defined(PNG_ALIGNED_MEMORY_SUPPORTED) > >>>> # define PNG_ARM_NEON_OPT 2 > >>>> # else > >>>> # define PNG_ARM_NEON_OPT 0 > >>>> # endif > >>>> #endif > >>>> > >>>> So, if we just predefine PNG_ARM_NEON=0 in the build this will have the > >>>> same effect as adding defined(__arm__) or !defined(__aarch64__) above. > >>>> > >>>> The following patch webrev does this:- > >>>> > >>>> http://cr.openjdk.java.net/~enevill/8078245/webrev.01/ > >>>> > >>>> If you are happy with this and if I could have another reviewer I will > >>>> push this. > >>>> > >>>> All the best, > >>>> Ed. >
