On 2015-06-02 12:27, David Holmes wrote:
On 2/06/2015 8:22 PM, Magnus Ihse Bursie wrote:
On 2015-06-02 11:57, David Holmes wrote:
On 2/06/2015 6:58 PM, Magnus Ihse Bursie wrote:
On 2015-06-01 19:23, Bertrand Delsart wrote:
Hi all,
A small open webrev to fix some issues around
--disable-warnings-as-errors (this is not a full cleanup):
http://cr.openjdk.java.net/~bdelsart/8081616/webrev.00/webrev/
Without the fix, -Werror is used to compiled native JDK libraries
even
when the --disable-warnings-as-errors is used.
A lot of these libs are addressed by clearing
CFLAGS_WARNINGS_ARE_ERRORS in flags.m4
Lib-jdk.sctp.gmk is a special case because it uses directly -Werror.
Instead of modifying SCTP to use CFLAGS_WARNINGS_ARE_ERRORS, I use
the
mechanism put in place in that file to disable -Werror and control it
through the WARNINGS_AS_ERRORS configured value.
I think this patch will require some work to be OK.
First of all, I'm not quite sure I understand what problem you are
trying to solve. What libraries are compiled with -Werror when
--disable-warnings-as-errors is used? This should not be the case,
and I
cannot reproduce it on my system.
CFLAGS_WARNINGS_ARE_ERRORS should never be cleared, since that is only
exported as the mechanism to use for enabling warnings as errors.
If no
compilation includes it (which it should not if warnings-as-errors are
disabled), it won't affect the build. If CFLAGS_WARNINGS_ARE_ERRORS
are
used even though warnings-as-errors are disabled, then it is this
usage
that should be fixed.
AFAICS most uses of CFLAGS_WARNINGS_ARE_ERRORS are unconditional eg:
$(eval $(call SetupNativeCompilation,BUILD_LIBINSTRUMENT, \
LIBRARY := instrument, \
OUTPUT_DIR := $(INSTALL_LIBRARIES_HERE), \
SRC := $(LIBINSTRUMENT_SRC), \
OPTIMIZATION := LOW, \
CFLAGS := $(LIBINSTRUMENT_CFLAGS) $(CFLAGS_WARNINGS_ARE_ERRORS), \
So clearing it is the simplest way to avoid it being applied.
Ah, I see. It might be the simplest but it's not correct. :-) Arguably,
these cases should have been removed when the global warnings-as-errors
functionality was introduced.
I count to 9 instances (including the incorrect usage in
Lib-jdk.jdwp.agent.gmk) of excplicitely hard-coding
CFLAGS_WARNINGS_ARE_ERRORS:
make/lib/Lib-jdk.jdi.gmk: CFLAGS := $(CFLAGS_JDKLIB)
$(CFLAGS_WARNINGS_ARE_ERRORS) -DUSE_MMAP \
make/lib/Lib-jdk.hprof.agent.gmk: CFLAGS := $(CFLAGS_JDKLIB)
$(CFLAGS_WARNINGS_ARE_ERRORS) \
make/lib/Lib-jdk.hprof.agent.gmk: CFLAGS := $(CFLAGS_JDKLIB)
$(CFLAGS_WARNINGS_ARE_ERRORS) \
make/lib/Lib-java.instrument.gmk: CFLAGS := $(LIBINSTRUMENT_CFLAGS)
$(CFLAGS_WARNINGS_ARE_ERRORS), \
make/lib/Lib-jdk.management.gmk: CFLAGS := $(CFLAGS_JDKLIB)
$(CFLAGS_WARNINGS_ARE_ERRORS) $(LIBMANAGEMENT_EXT_CFLAGS), \
make/lib/Lib-jdk.attach.gmk: CFLAGS := $(CFLAGS_JDKLIB)
$(CFLAGS_WARNINGS_ARE_ERRORS) \
make/lib/Lib-jdk.jdwp.agent.gmk: CFLAGS := $(CFLAGS_JDKLIB)
$(CFLAGS_CFLAGS_WARNINGS_ARE_ERRORS) -DUSE_MMAP \
make/lib/Lib-jdk.jdwp.agent.gmk: CFLAGS := $(CFLAGS_JDKLIB)
$(CFLAGS_WARNINGS_ARE_ERRORS) -DJDWP_LOGGING \
make/lib/Lib-java.management.gmk: CFLAGS := $(CFLAGS_JDKLIB)
$(CFLAGS_WARNINGS_ARE_ERRORS) $(LIBMANAGEMENT_CFLAGS), \
These should be removed instead.
Okay but I don't think it reasonable to expect Bertrand to make such
changes. In the interest of moving forward can we use his current
patch and file a follow up bug to get this done properly by the build
team?
It's not worth the trouble to put in bad code just to rip it out. I'll
have the patch done in five minutes, brb. :)
/Magnus
Thanks,
David
-----
I also checked for -Werror in the code, but could only find the already
discovered location in Lib-jdk.sctp.gmk. For that, I believe the proper
solution is:
diff --git a/make/lib/Lib-jdk.sctp.gmk b/make/lib/Lib-jdk.sctp.gmk
--- a/make/lib/Lib-jdk.sctp.gmk
+++ b/make/lib/Lib-jdk.sctp.gmk
@@ -30,12 +30,8 @@
ifeq ($(OPENJDK_TARGET_OS_TYPE), unix)
ifeq (, $(filter $(OPENJDK_TARGET_OS), macosx aix))
-
- # Suppress unused parameters required by exported JNI functions.
- SCTP_WERROR := -Werror -Wno-error=unused-parameter
- ifeq ($(OPENJDK_TARGET_CPU_ARCH), ppc)
- SCTP_WERROR :=
- endif
+ # DISABLED_WARNINGS_gcc := unusused-parameter needed to
+ # suppress unused parameters required by exported JNI functions.
$(eval $(call SetupNativeCompilation,BUILD_LIBSCTP, \
LIBRARY := sctp, \
@@ -49,7 +45,7 @@
$(LIBJAVA_HEADER_FLAGS) \
-I$(SUPPORT_OUTPUTDIR)/headers/jdk.sctp \
-I$(SUPPORT_OUTPUTDIR)/headers/java.base, \
- CFLAGS_linux := $(SCTP_WERROR), \
+ DISABLED_WARNINGS_gcc := unusused-parameter, \
MAPFILE := $(JDK_TOPDIR)/make/mapfiles/libsctp/mapfile-vers, \
LDFLAGS := $(LDFLAGS_JDKLIB) \
$(call SET_SHARED_LIBRARY_ORIGIN), \
However, as I said, it should probably be verified that it is correct
that the unused-parameter warning is still triggered.
/Magnus
David
-----
I can see that libsctp is a special case that hard-codes -Werror.
But in
this case we should remove the hard-coding and relying on the system
setting. This is probably a remnant from before the overall -Werror
usage, where the authors of a specific lib wanted to enforce a higher
standard. Also, it might be worth revisiting if
-Wno-error=unused-parameter is really needed. These things tend to
bit-rot.
/Magnus
Best regards,
Bertrand.
