I'm not sure I'm able to follow where this discussion is heading, but at the very least I'd like to chime in on the patch below:

I don't like how the entire DISABLED_WARNINGS_gcc line is lifted out. A better solution is something like this:

ifeq ($(CC_VERSION_NUMBER), 4.8.5)
 #Turn off array-bounds warnings for libjava compilation because jchuff.c
 #fails to compile on gcc 4.8.5 with a warning that an array is potentially
 #out of bounds. This kills a warnings=errors build.
 LIBJAVAJPEG_DISABLED_WARNINGS_gcc485 := array-bounds
endif
...
    DISABLED_WARNINGS_gcc := clobbered $(LIBJAVAJPEG_DISABLED_WARNINGS_gcc485), \

I also can't see this going into anything else than the current mainline, jdk/jdk i.e. JDK 11. Since the issue is trivially avoided by using --disable-warnings-as-errors, I hardly see that it warrants a backport.

/Magnus

On 2018-01-23 14:44, Adam Farley8 wrote:
Hi All,

All: I think I responded to everyone below. Please could a committer or
author raise a bug and, if people are happy with this change, line it up
for contribution to JDKs 8-11 (assuming 4.8.5 is still the recommended gcc
for JDK10 and 11 on SUSE sles)?

Erik: One toolchain-specific change, as requested. This was tested on JDK9, so please apply an offset of -1 to the line numbers before applying to JDK10.

----------------------- Start of Diff -----------------------

diff --git a/make/lib/Awt2dLibraries.gmk b/make/lib/Awt2dLibraries.gmk
--- a/make/lib/Awt2dLibraries.gmk
+++ b/make/lib/Awt2dLibraries.gmk
@@ -482,6 +482,14 @@
   BUILD_LIBJAVAJPEG_HEADERS := $(addprefix -I, $(LIBJAVAJPEG_SRC))
 endif

+LIBJAVAJPEG_DISABLED_WARNINGS_gcc := clobbered
+ifeq ($(CC_VERSION_NUMBER), 4.8.5)
+  #Turn off array-bounds warnings for libjava compilation because jchuff.c +  #fails to compile on gcc 4.8.5 with a warning that an array is potentially
+  #out of bounds. This kills a warnings=errors build.
+  LIBJAVAJPEG_DISABLED_WARNINGS_gcc := clobbered array-bounds
+endif
+
 $(eval $(call SetupNativeCompilation,BUILD_LIBJAVAJPEG, \
     LIBRARY := javajpeg, \
     OUTPUT_DIR := $(INSTALL_LIBRARIES_HERE), \
@@ -491,7 +499,7 @@
     CFLAGS := $(CFLAGS_JDKLIB) $(BUILD_LIBJAVAJPEG_HEADERS) \
         $(LIBJAVA_HEADER_FLAGS) \
 -I$(SUPPORT_OUTPUTDIR)/headers/java.desktop, \
-    DISABLED_WARNINGS_gcc := clobbered, \
+    DISABLED_WARNINGS_gcc := $(LIBJAVAJPEG_DISABLED_WARNINGS_gcc), \
     MAPFILE := $(JDK_TOPDIR)/make/mapfiles/libjpeg/mapfile-vers, \
     LDFLAGS := $(LDFLAGS_JDKLIB) \
         $(call SET_SHARED_LIBRARY_ORIGIN), \

----------------------- End of Diff -----------------------

Phil: I've changed the title as asked and supplied the diff above. However, I can't upload files to cr.openjdk.java.net, nor can I create bugs myself.
I understand that authors and committers can, but I don't have those
privileges yet. Be nice if I did. :)

John: I read your email, and I understand your position. I disagree with it, but I understand it. 4.8.5 is an old version of gcc, but right now it is the listed gcc version for SUSE sles on intel, ppc, ppcle, and zLinux. Even if
this is not the case for JDK 10 or 11, we should ensure this fix is fully
propagated to ensure consistent behaviour.

That is my position.

David: Thanks for the URL. I agree with your position on 4.8.5 gcc needing
to compile OpenJDK without errors or special options. :)

Best Regards

Adam Farley



From: Phil Race <philip.r...@oracle.com>
To: Adam Farley8 <adam.far...@uk.ibm.com>, 2d-...@openjdk.java.net
Cc: build-dev <build-dev@openjdk.java.net>
Date: 18/01/2018 19:16
Subject: Re: [OpenJDK 2D-Dev] [PATCH] Build fails to compile jchuff.c
------------------------------------------------------------------------



Try again with build-dev cc'd ..

-phil.

On 01/18/2018 11:14 AM, Phil Race wrote:
I agree with what Erik said on build-dev that being specific about the tool chain and the reason are worthwhile and important. We've done that in similar cases.

Also these review threads usually should have a subject like
RFR: <BUG ID>: <Bug Synopsis>

which means you first need a bug id .. the patch can't be pushed without one anyway.

Then the patch should be an in-line diff or a webrev hosted on cr.openjdk.java.net.

I think in-line would be OK for this small change.

-phil.

On 01/17/2018 09:30 AM, Adam Farley8 wrote:
Hi All,

Under these circumstances, jchuff.c will not compile:

Platform: zLinux (s390x)
Release: JDK9 (may affect other JDKs).
GCC Version: 4.8.5
Notes: --disable-warnings-as-errors suppresses this error.

The error is:

/home/adamfarl/hotspot/jdk9/jdk/src/java.desktop/share/native/libjavajpeg/jchuff.c:
In function 'jGenOptTbl':
/home/adamfarl/hotspot/jdk9/jdk/src/java.desktop/share/native/libjavajpeg/jchuff.c:808:18:
error: array subscript is below array bounds [-Werror=array-bounds]
      while (bits[j] == 0)
                 ^

This is a continuation of a conversation in the build-dev mailing list, if anyone wants to
check the history.

The short version is that, while you *can* suppress the problem by adding
--disable-warnings-as-errors to your configure step, I posit that a builder shouldn't
have to.

Various solutions were debated. One involves changing Awt2dLibraries.gmk.

Basically you change line 494 to this:

    DISABLED_WARNINGS_gcc := clobbered array-bounds, \

I'm running a build now to check that works, but basically we should end up with a -Wno-array-bounds on the gcc compile command for jchuff.c, thereby ignoring the warning.

A smarter variant involves checking for that specific version of the gcc, but that seems
wordy to me for this problem. Keeping it simple. :)

Thoughts?

Best Regards

Adam Farley

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU





Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Reply via email to