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

[Igor Ignatyev]

Hi Magnus,

herein I will be talking only about 1st two warnings.
 
although your analyze is correct, it doesn't take into account the fact that the warnings report situations that can't happen in current codebase, and gcc doesn't report them in our "regular" builds b/c it can proof that
- in macroAssembler_x86.cpp both rounding_control() and precision_control() are >= 0 and <= 3, so switch statements do cover all possible cases
- in genCollectedHeap.cpp, removeSmallestScratch is called only from one place w/ prev_ptr being an address of local variable, therefore we will always visit while loop, so smallest_ptr and smallest will be inited.

so these warnings are false-possitive b/c they can never happen. one can argue that if we change the code, these issues might become real. that's true, but when it happens, gcc will report them as warnings for non-instrumented builds, as the compiler won't be able to proof that prev_ptr is always not null, or precision_control is from [0;3], etc. this is one of the reasons why these bugs won't be ever treated the same as other compiler errors. they are false-possitive from practical point of view and fixing them not always improve code quality, it some case we might end up w/ more clumsy code just to fix "potential" issues w/ zero probability to happen. if we want to use compiler warnings to improve quality, I'd suggest us to start w/ removing currently disabled warnings and/or start using clang static analyzer and clang tidy rather than fixing issues which will never happen.

disabling warnings in instrumented builds isn't the same as disabling all warnings in a single stroke, it's just disabling warnings in a build configuration we don't treat as important as other configurations.

as for the bug I posted, it was "fixed" by passing '--disable-warnings-as-errors' to jprt if native-coverage is enabled.

Thanks,
-- Igor

> On Aug 31, 2018, at 9:21 AM, Erik Joelsson <erik.joelsson at oracle.com> wrote:
> 
> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20180831/0142ef0d/attachment.html>