RFR: 8065585: Change ShouldNotReachHere() to never return

[Kim Barrett]

On Apr 15, 2015, at 6:49 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> We might want to change the the behavior of Unimplemented() and Untested(...), but they are used a lot in compiler code, so I've not changed them for this patch. There has been request to leave guarantee(...) unchanged so that they can be turned off in production code.
> 
> I had to change some instance of ShouldNotReachHere() in destructors, because the VS C++ compiler complained about potential memory leaks.
> 
> http://cr.openjdk.java.net/~stefank/8065585/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-8065585

Nice!  A few comments:

------------------------------------------------------------------------------  
> There has been request to leave guarantee(...) unchanged so that they
> can be turned off in production code.

Isn't that what assert() is for?

I'm not sure whether guarantee() should use noreturn or not.  Not, for
consistency with assert() and for the same reasons has some appeal.
On the other hand, guarantee() is supposed to be for use in production
contexts.

I'm wondering if guarantee() might be one (and quite possibly the
only) place where we might consider making the use of noreturn
conditional.  This would make use of guarantee() is little trickier,
e.g. guarantee(false, ...) is probably not sensible with that sort of
configuration.  But we ought to be using fatal in such a situation
anyway.

------------------------------------------------------------------------------   
Many missing copyright updates.

------------------------------------------------------------------------------   
src/cpu/x86/vm/x86_32.ad
1212   Unimplemented();
1213   return 0; // Mute compiler
=>
1212   fatal("Unimplemented");

Why was this changed?  The email describing the change makes me think
this shouldn't be part of this change set.

------------------------------------------------------------------------------   
src/share/vm/gc_implementation/g1/g1CollectorPolicy.hpp
 895         fatal(err_msg("Unknown dest state: " CSETSTATE_FORMAT, dest.value()));
 896         break;

Shouldn't the "break" also be unnecessary here?

------------------------------------------------------------------------------  
src/share/vm/gc_implementation/g1/g1StringDedupThread.cpp
  42   guarantee(false, "Should never be destroyed");

guarantee => fatal.  

------------------------------------------------------------------------------ 
src/share/vm/utilities/debug.hpp 

I think some commentary explaining why assert() and friends do *not*
use the noreturn form of error error reporting might be in order.  I'm
guessing it is to improve the debugging experience for fastdebug
builds; if assert used noreturn reporting then interesting values
might be dead and optimized away, making them inaccessible in the
debugger.

------------------------------------------------------------------------------
src/share/vm/utilities/hashtable.hpp
  62   BasicHashtableEntry()  { guarantee(false, "Should not be called"); }
...
  65   ~BasicHashtableEntry() { guarantee(false, "Should not be called"); }

guarantee => fatal. 

And by the way, this whole mechanism invokes undefined behavior.  I
suspect someone didn't understand placement new and delete.  But
that's not a problem with this change set.

------------------------------------------------------------------------------