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

Stefan Karlsson stefan.karlsson at oracle.com
Thu Apr 16 06:11:08 UTC 2015

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/x8632.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/gcimplementation/g1/g1CollectorPolicy.hpp 895 fatal(errmsg("Unknown dest state: " CSETSTATEFORMAT, dest.value())); 896 break; Shouldn't the "break" also be unnecessary here?

Probably. I'll test.

------------------------------------------------------------------------------ src/share/vm/gcimplementation/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

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

More information about the hotspot-dev mailing list