<AWT Dev> 8209520: Build fails when native code coverage is enabled

[Leonid Mesnik]

Hi

I read 
>>>>  It’d be much better and reliable to fix makefiles to always use ‘disable-warning-as-errors’ when ‘enable-native-coverage’ is used. It should be pretty straightforward to do.

as you propose not to fix false positive warnings but fix make files instead. Because warnings in macroAssembler_x86 and genCollectedHeap  are false positive. gcc without -fno-profile  have enough information to verify that variables are initialized and don't produce warning. So my fixes doesn't make code much better. 

Leonid
> On Aug 30, 2018, at 11:36 AM, Igor Ignatyev <igor.ignatyev at oracle.com> wrote:
> 
> Hi,
> 
> my claim was based on the warnings which we were getting when we just introduced code coverage builds in JDK 9, e.g. 8130790[1] (clobbered warning in libt2k). these warnings haven't been seen w/o code coverage enabled, and enabling coverage changes code path, so I don't think we should care much about these warnings.
> 
> I ain't saying that Leonid's fixes are wrong, I definitely support his changes in macroAssembler_x86 and genCollectedHeap (can't comment on splashscreen_png.c as I don't really understand what gcc complains there), I'm saying that b/c coverage-enabled builds aren't built often enough and warnings from these builds will always be low-priority bugs, I believe that such instrumented must be produced w/ warnings-as-errors disabled.
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8130790
> 
> Thanks,
> -- Igor
> 
> 
>> On Aug 30, 2018, at 6:26 AM, Erik Joelsson <erik.joelsson at oracle.com> wrote:
>> 
>> Hello,
>> 
>> On 2018-08-30 02:22, Magnus Ihse Bursie wrote:
>>> 
>>> 
>>> On 2018-08-24 00:44, Igor Ignatev wrote:
>>>> Hi Leonid,
>>>> 
>>>> We have never supported native code coverage builds with warnings enabled as errors. There are bugs in gcc which cause false positive warnings, so it was decided to ignore all warnings from instrumented builds. It’d be much better and reliable to fix makefiles to always use ‘disable-warning-as-errors’ when ‘enable-native-coverage’ is used. It should be pretty straightforward to do.
>>> I disagree.
>>> 
>>> While it is simple to change the build to disable warnings as error when building with code coverage, I think hard-coding this into the build system is a step backwards :-( and not something I want to support.
>>> 
>> I shared your opinion at first while discussing this offline with Leonid. What changed my mind was the claim that the warnings cannot be truly trusted when GCC is generating code coverage data. If that is true, then having warnings as errors turned on then serves no purpose. The majority of builds will be without code coverage enabled, so we will still get ample protection against warnings in the code.
>>> The code changes suggested by Leonid seems trivial and benign, and helps readability, even apart from fixing compiler warnings.
>>> 
>>> If this is deemed unacceptable, it's better to disable those few warnings specifically, for the files/libraries they occur in.
>>> 
>> If the claim on the warnings produced by GCC while generating code coverage is false, then I certainly agree that the warnings should be fixed instead.
>> 
>> /Erik
>>> /Magnus
>>> 
>>> 
>>>> 
>>>> cc’ing build alias.
>>>> 
>>>> Cheers,
>>>> — Igor
>>>> 
>>>>> On Aug 23, 2018, at 2:37 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>>> 
>>>>> macroassembler changes are good.
>>>>> 
>>>>> Thanks,
>>>>> Vladimir
>>>>> 
>>>>>> On 8/23/18 1:51 PM, Leonid Mesnik wrote:
>>>>>> Hi
>>>>>> Could you please review following fix which fix code so gcc doesn't complain when JDK is build with enabled native code coverage.
>>>>>> webrev: http://cr.openjdk.java.net/~lmesnik/8209520/webrev.00/ <http://cr.openjdk.java.net/%7Elmesnik/8209520/webrev.00/>
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8209520
>>>>>> These warning appeared because of change optimization settings used for getting code coverage.
>>>>>> 1) src/hotspot/cpu/x86/macroAssembler_x86.cpp, src/hotspot/share/gc/shared/genCollectedHeap.cpp
>>>>>> gcc complained about uninitialized variables, like
>>>>>> * For target hotspot_variant-server_libjvm_objs_macroAssembler_x86.o:
>>>>>> /home/lmesnik/ws/jdk-8209520/open/src/hotspot/cpu/x86/macroAssembler_x86.cpp: In member function 'void ControlWord::print() const':
>>>>>> /home/lmesnik/ws/jdk-8209520/open/src/hotspot/cpu/x86/macroAssembler_x86.cpp:5769:11: error: 'pc' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>>      printf("%04x  masks = %s, %s, %s", _value & 0xFFFF, f, rc, pc);
>>>>>> ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>> /home/lmesnik/ws/jdk-8209520/open/src/hotspot/cpu/x86/macroAssembler_x86.cpp:5769:11: error: 'rc' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>> So I just fixed codepath to show more explicitly that variables are initialized before usage.
>>>>>> 2) src/java.desktop/share/native/libsplashscreen/splashscreen_png.c:
>>>>>> The changes to prevent waning about clobbering in splashscreen_png.c are similar to fix in:
>>>>>> 1. JDK-8080695 <https://bugs.openjdk.java.net/browse/JDK-8080695>
>>>>>>    splashscreen_png.c compile error with gcc 4.9.2
>>>>>> The another approach would be to fix build to ignore these warnings for code coverage build. While I think it makes build system even more complicated.
>>>>>> Leonid
>>> 
>> 
>