On 2018-08-31 01:20, Magnus Ihse Bursie wrote:
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.
I agree and it seems I was assuming too much when I claimed 1.
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.
Thank you for the detailed analysis. With this I definitely agree that
it's better to fix the code.
/Erik
/Magnus
