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

Tue Jun 2 10:33:37 UTC 2015

On 02/06/2015 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?

Thanks David,

I had written an answer but yours is more to the point :-)

Stated differently, is there any issue with my proposed fixes except the 
fact that, while sufficient for our current issue, it does not fully 
clean up what started to bit-rot ?

Do I need extra reviewers from the build-team ?

Regards,

Bertrand.

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

-- 
Bertrand Delsart,                     Grenoble Engineering Center
Oracle,         180 av. de l'Europe,          ZIRST de Montbonnot
38330 Montbonnot Saint Martin,                             FRANCE
bertrand.delsart at oracle.com             Phone : +33 4 76 18 81 23

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged
information. Any unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient,
please contact the sender by reply email and destroy all copies of
the original message.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~