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

Magnus Ihse Bursie magnus.ihse.bursie at oracle.com
Fri Aug 31 08:20:54 UTC 2018


There are two aspects here that sound similar, but is not:

Erik says:
> On Aug 30, 2018, at 6:26 AM, Erik Joelsson <erik.joelsson at oracle.com> wrote:
>
> 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.

and Igor says:

Hi,
> On 2018-08-30 20:36, Igor Ignatyev wrote:
>
> 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.
>

These are two different arguments for turning off warnings for code 
coverage:
1) gcc is producing incorrect warnings
2) the warnings might be correct, but we are going to treat such bugs as 
low priority


I understand and accept 1, but I do not accept 2 as a valid reason for 
turning off warnings. I tried to dig through history in the 
(Oracle-internal) bug Igor posted, but in the end I could find no 
changes was made to any compiler flags..?

I have googled around but found no discussions at all that indicate that 
gcov should emit invalid warnings.

As for argument 2: we're using code coverage as a way to increase code 
quality, after all. If we get additional warnings, that indicate ways to 
improve the code, then we should use this to improve the code, not hide 
it away. And there's no reason not to treat such bugs as severe as any 
other compile errors.

Let's get more concrete: In this case, we have three warnings. To solve 
this, Leonid fixed:

* In macroAssembler_x86.cpp, he added ShouldNotReachHere(), which 
definitely improves code quality.

* In genCollectedHeap.cpp, he he initalized two local variables to NULL. 
After studying the code, I see that this is strictly not needed, and gcc 
is apparently losing some information here that could have been used to 
determine that the code is correct. However, my initial reaction when 
reading the code was that it was indeed broken, and it took me some time 
to prove that it was not. This is not good code readability. It's 
fragile, and some future code change might end up with a non-initialized 
local variable. So I think this is good, defensive programming, no 
matter what.

* In splashscreen_png.c, I'll honestly say that I don't know wether this 
fix is correct or not. But it does seem reasonable. The 
SplashDecodePng() function uses setjmp() *shudder* and for me, that's a 
big Here Be Dragons! sign. I also note that the success variable is read 
in the done: label, just below access to row_pointers and image_data -- 
which are both, already, declared volatile. So I'm guessing that 
declaring success volatile is in fact the correct thing to do. (I 
challenge anyone with a greater understanding of setjmp to prove me 
wrong...)

So I say we should be happy we got those warnings, and fix the code.

I don't see this as an argument that gcc emits invalid warnings. If that 
happens in the future, then we can discuss eliminating those warning. 
But if we do, we should eliminate them selectively, only the incorrect 
warnings and only on the files/libraries where they occur erroneously. 
Not disable all warnings in a single stroke.

/Magnus




More information about the awt-dev mailing list