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

[Lindenmaier, Goetz]

Hi Roland,

thanks for reviewing this change!

I changed the assert to fatal, I'm fine with that, too:
http://cr.openjdk.java.net/~goetz/webrevs/8139040-init/webrev.04/

Coleen, Roland, would one of you mind sponsoring this?

Best regards,
  Goetz.

-----Original Message-----
From: Roland Westrelin [mailto:roland.westrelin at oracle.com] 
Sent: Dienstag, 13. Oktober 2015 10:10
To: Lindenmaier, Goetz
Cc: hotspot-compiler-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR(M): 8139040: Fix initializations before ShouldNotReachHere()

Hi Goetz,

> C1_LIRGenerator.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 get_num 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(max_prev != NULL).  I think this is better understandable.
> Else it looks as if there is a legal case where *max_prev 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/c1_LIRGenerator.hpp.sdiff.html
> 
> handle default case? I dont see callers handling lir_cond_unknown.
> 
> ------------
> 
> 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::get_num() ?
> (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::delete_region_table():
> 
> 617   // Unsplice.
> 618   *max_prev = max->collision_list_next();
> 
> Should the access to max_prev 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.