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

[Magnus Ihse Bursie]

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