Hi I read >>>> 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.
as you propose not to fix false positive warnings but fix make files instead. Because warnings in macroAssembler_x86 and genCollectedHeap are false positive. gcc without -fno-profile have enough information to verify that variables are initialized and don't produce warning. So my fixes doesn't make code much better. Leonid > On Aug 30, 2018, at 11:36 AM, Igor Ignatyev <igor.ignat...@oracle.com> wrote: > > 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 >>> >> >
