<html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hi Magnus,<div class=""><br class=""></div><div class="">herein I will be talking only about 1st two warnings.</div><div class=""> </div><div class="">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</div><div class="">- in macroAssembler_x86.cpp both rounding_control() and precision_control() are >= 0 and <= 3, so switch statements do cover all possible cases</div><div class="">- 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.</div><div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class="">as for the bug I posted, it was "fixed" by passing '--disable-warnings-as-errors' to jprt if native-coverage is enabled.</div><div class=""><br class=""></div><div class="">Thanks,</div><div class="">-- Igor</div><div class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Aug 31, 2018, at 9:21 AM, Erik Joelsson <<a href="mailto:erik.joelsson@oracle.com" class="">erik.joelsson@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">On 2018-08-31 01:20, Magnus Ihse Bursie wrote:</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br class="">These are two different arguments for turning off warnings for code coverage:<br class="">1) gcc is producing incorrect warnings<br class="">2) the warnings might be correct, but we are going to treat such bugs as low priority<br class=""><br class=""><br class="">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..?<br class=""><br class="">I have googled around but found no discussions at all that indicate that gcov should emit invalid warnings.<br class=""><br class="">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.<br class=""><br class=""></blockquote><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">I agree and it seems I was assuming too much when I claimed 1.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class="">Let's get more concrete: In this case, we have three warnings. To solve this, Leonid fixed:<br class=""><br class="">* In macroAssembler_x86.cpp, he added ShouldNotReachHere(), which definitely improves code quality.<br class=""><br class="">* 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.<br class=""><br class="">* 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...)<br class=""><br class="">So I say we should be happy we got those warnings, and fix the code.<br class=""><br class="">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.<br class=""><br class=""></blockquote><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">Thank you for the detailed analysis. With this I definitely agree that it's better to fix the code.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">/Erik</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class="">/Magnus</blockquote></div></blockquote></div><br class=""></div></body></html>