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

[David Holmes]

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?

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