<AWT Dev> 8209520: Build fails when native code coverage is enabled
erik.joelsson at oracle.com
Fri Aug 31 16:21:41 UTC 2018
On 2018-08-31 01:20, Magnus Ihse Bursie wrote:
> These are two different arguments for turning off warnings for code
> 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.
More information about the awt-dev