RFR: 8065585: Change ShouldNotReachHere() to never return

[Stefan Karlsson]

On 2015-04-16 01:04, Kim Barrett wrote:
> 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?

Support wants to be able to turn of guarantees in a few cases where we 
actually don't have to shutdown the JVM if the guarantee is hit.
>
> 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.

Since guarantee can, and often will :), return it shouldn't be marked 
with noreturn. I was thinking about the guarantee(false, ...) use case.

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

Yes. I usually don't update copyrights when I send out my review requests.

>
> ------------------------------------------------------------------------------
> 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.

It might be a left-over from when I tried to change Unimplemented. I'll 
take a look at it.
>
> ------------------------------------------------------------------------------
> 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?

Probably. I'll test.

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

No. If I do that change, then the VS C++ compiler complains that we leak 
memory since we never return from the destructor. But maybe it's better 
to find a way to turn off that warning?

>
> ------------------------------------------------------------------------------
> 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.

You can return from assert, that's why it's not marked with noreturn.
>
> ------------------------------------------------------------------------------
> 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.

Same comment above about the memory leak.

Thanks,
StefanK
>
> ------------------------------------------------------------------------------
>