RFR [XXS] : JDK-8081616: build fixes for --disable-warnings-as-errors

[Magnus Ihse Bursie]

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.

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.
>>>
>>