RFR(M): 8139040: Fix initializations before ShouldNotReachHere() (original) (raw)

Roland Westrelin roland.westrelin at oracle.com
Tue Oct 13 08:10:00 UTC 2015

Hi Goetz,

C1LIRGenerator.hpp: I added handling of the default case (assert(0)).

Shouldn’t you use fatal instead of assert(0)? I went over the compiler related files and they look ok to me.

Roland.

compactHashtable.cpp: Hmm, I’m not clear why getnum says corrupted() in one case, and in the other returns false. The code is dead, anyways, so I rather leave it as is. heapRegionRemSet.cpp I added guarantee(maxprev != NULL). I think this is better understandable. Else it looks as if there is a legal case where *maxprev must not be set. Updated webrev: http://cr.openjdk.java.net/~goetz/webrevs/8139040-init/webrev.03/ Best regards, Goetz.

From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com] Sent: Montag, 12. Oktober 2015 16:45 To: Lindenmaier, Goetz Cc: hotspot-runtime-dev at openjdk.java.net Subject: Re: RFR(M): 8139040: Fix initializations before ShouldNotReachHere() Hi Goetz, More small nitpicks: http://cr.openjdk.java.net/~goetz/webrevs/8139040-init/webrev.02/src/share/vm/c1/c1LIRGenerator.hpp.sdiff.html handle default case? I dont see callers handling lircondunknown. ------------ http://cr.openjdk.java.net/~goetz/webrevs/8139040-init/webrev.02/src/share/vm/classfile/compactHashtable.cpp.html maybe the right thing to do would be to handle errors returned by HashTableHexDump::getnum() ? (but your change is still better than the original) -------------- http://cr.openjdk.java.net/~goetz/webrevs/8139040-init/webrev.02/src/share/vm/gc/g1/heapRegionRemSet.cpp.sdiff.html In PerRegionTable* OtherRegionsTable::deleteregiontable(): 617 // Unsplice. 618 *maxprev = max->collisionlistnext(); Should the access to maxprev be guarded against != NULL? ------------ Otherwise the change looks good. Kind Regards, Thomas On Mon, Oct 12, 2015 at 11:18 AM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com<mailto:goetz.lindenmaier at sap.com>> wrote: Hi, I now fixed the warnings showing with -Wuninitialized anyways. The change now enables this flag per default with gcc 4.8. Older gcc versions detect more issues, as far as I checked all false positives. Therefore I don't enable the flag for gcc < 4.8._ _The warnings reported by this flag depend on the optimization level, it's_ _most effective in the opt build._ _This unveiled some missing initializations in constructors, as in compile.hpp,_ _and oopMap.hpp that can actually cause wrong behavior._ _Complete webrev:_ _http://cr.openjdk.java.net/~goetz/webrevs/8139040-init/webrev.02/_ _incremental webrev:_ _http://cr.openjdk.java.net/~goetz/webrevs/8139040-init/webrev.02-incremental/_ _Best regards,_ _Goetz._ _From: Lindenmaier, Goetz_ _Sent: Mittwoch, 7. Oktober 2015 15:22_ _To: hotspot-runtime-dev at openjdk.java.net<mailto:hotspot-runtime-dev at openjdk.java.net> Subject: RFR(M): 8139040: Fix initializations before ShouldNotReachHere() Hi, SAP requires us to fix a row of issues in the hotspot coding. I would like to share these with openJDK. This webrev fixes a row of missing intializations, mostly combined with ShouldNotReachHere() in default cases of switches or the like. http://cr.openjdk.java.net/~goetz/webrevs/8139040-init/webrev.00/ In the debug build, ShouldNotReachHere() can be suppressed, so the uninitialized value actually can cause problems. In opt builds, not all tools recognize the ShouldNotReachHere properly. In addition to this I would like to add -Wuninitialized to the warning flags. This finds most of these issues in the opt build and would require an additional 70 fixes plus fixes in jvmtiEnter.xsl. Would it be appreciated to set this flag? Best regards, Goetz.

More information about the hotspot-runtime-dev mailing list