Hi, my claim was based on the warnings which we were getting when we just introduced code coverage builds in JDK 9, e.g. 8130790[1] (clobbered warning in libt2k). these warnings haven't been seen w/o code coverage enabled, and enabling coverage changes code path, so I don't think we should care much about these warnings.
I ain't saying that Leonid's fixes are wrong, I definitely support his changes in macroAssembler_x86 and genCollectedHeap (can't comment on splashscreen_png.c as I don't really understand what gcc complains there), I'm saying that b/c coverage-enabled builds aren't built often enough and warnings from these builds will always be low-priority bugs, I believe that such instrumented must be produced w/ warnings-as-errors disabled. [1] https://bugs.openjdk.java.net/browse/JDK-8130790 Thanks, -- Igor > On Aug 30, 2018, at 6:26 AM, Erik Joelsson <erik.joels...@oracle.com> wrote: > > Hello, > > On 2018-08-30 02:22, Magnus Ihse Bursie wrote: >> >> >> On 2018-08-24 00:44, Igor Ignatev wrote: >>> Hi Leonid, >>> >>> We have never supported native code coverage builds with warnings enabled >>> as errors. There are bugs in gcc which cause false positive warnings, so it >>> was decided to ignore all warnings from instrumented builds. It’d be much >>> better and reliable to fix makefiles to always use >>> ‘disable-warning-as-errors’ when ‘enable-native-coverage’ is used. It >>> should be pretty straightforward to do. >> I disagree. >> >> While it is simple to change the build to disable warnings as error when >> building with code coverage, I think hard-coding this into the build system >> is a step backwards :-( and not something I want to support. >> > I shared your opinion at first while discussing this offline with Leonid. > What changed my mind was the claim that the warnings cannot be truly trusted > when GCC is generating code coverage data. If that is true, then having > warnings as errors turned on then serves no purpose. The majority of builds > will be without code coverage enabled, so we will still get ample protection > against warnings in the code. >> The code changes suggested by Leonid seems trivial and benign, and helps >> readability, even apart from fixing compiler warnings. >> >> If this is deemed unacceptable, it's better to disable those few warnings >> specifically, for the files/libraries they occur in. >> > If the claim on the warnings produced by GCC while generating code coverage > is false, then I certainly agree that the warnings should be fixed instead. > > /Erik >> /Magnus >> >> >>> >>> cc’ing build alias. >>> >>> Cheers, >>> — Igor >>> >>>> On Aug 23, 2018, at 2:37 PM, Vladimir Kozlov <vladimir.koz...@oracle.com> >>>> wrote: >>>> >>>> macroassembler changes are good. >>>> >>>> Thanks, >>>> Vladimir >>>> >>>>> On 8/23/18 1:51 PM, Leonid Mesnik wrote: >>>>> Hi >>>>> Could you please review following fix which fix code so gcc doesn't >>>>> complain when JDK is build with enabled native code coverage. >>>>> webrev: http://cr.openjdk.java.net/~lmesnik/8209520/webrev.00/ >>>>> <http://cr.openjdk.java.net/%7Elmesnik/8209520/webrev.00/> >>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8209520 >>>>> These warning appeared because of change optimization settings used for >>>>> getting code coverage. >>>>> 1) src/hotspot/cpu/x86/macroAssembler_x86.cpp, >>>>> src/hotspot/share/gc/shared/genCollectedHeap.cpp >>>>> gcc complained about uninitialized variables, like >>>>> * For target hotspot_variant-server_libjvm_objs_macroAssembler_x86.o: >>>>> /home/lmesnik/ws/jdk-8209520/open/src/hotspot/cpu/x86/macroAssembler_x86.cpp: >>>>> In member function 'void ControlWord::print() const': >>>>> /home/lmesnik/ws/jdk-8209520/open/src/hotspot/cpu/x86/macroAssembler_x86.cpp:5769:11: >>>>> error: 'pc' may be used uninitialized in this function >>>>> [-Werror=maybe-uninitialized] >>>>> printf("%04x masks = %s, %s, %s", _value & 0xFFFF, f, rc, pc); >>>>> ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>> /home/lmesnik/ws/jdk-8209520/open/src/hotspot/cpu/x86/macroAssembler_x86.cpp:5769:11: >>>>> error: 'rc' may be used uninitialized in this function >>>>> [-Werror=maybe-uninitialized] >>>>> So I just fixed codepath to show more explicitly that variables are >>>>> initialized before usage. >>>>> 2) src/java.desktop/share/native/libsplashscreen/splashscreen_png.c: >>>>> The changes to prevent waning about clobbering in splashscreen_png.c are >>>>> similar to fix in: >>>>> 1. JDK-8080695 <https://bugs.openjdk.java.net/browse/JDK-8080695> >>>>> splashscreen_png.c compile error with gcc 4.9.2 >>>>> The another approach would be to fix build to ignore these warnings for >>>>> code coverage build. While I think it makes build system even more >>>>> complicated. >>>>> Leonid >> >
