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

[Erik Joelsson]

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
>
>