RFR: 8065585: Change ShouldNotReachHere() to never return (original) (raw)

Kim Barrett kim.barrett at oracle.com
Wed Apr 15 23:04:46 UTC 2015

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.

More information about the hotspot-dev mailing list