<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 build-dev
mailing list