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

Stefan Karlsson stefan.karlsson at oracle.com
Thu Apr 16 12:07:27 UTC 2015

Hi David,

On 2015-04-16 04:23, David Holmes wrote:

Hi Stefan,

Stefan Karlsson wrote: Hi,

Today the it's possible for the code to return out from ShouldNotReachHere() calls. This sometimes forces us to add return statements and assignments to parts of the code where it they don't make sense. By telling the compiler that ShouldNotReachHere is a dead end, we can get rid of these unnecessary constructs. For example: 1) We could get rid of return statements after ShouldNotReachHere(): bool ismarked() { // actual code here // Execution path that "should not" happen. ShouldNotReachHere(); return false; } 2) The following code will actually use an uninitialized value today. The compiler will find this if we turn on -Wuninitialized. But if we change ShouldNotReachHere() to not return, the compiler will stop complaining because report(type) will never be called with an uninitialized value: int type; switch (value) { case TYPEOOP: type = 0; break; case TYPEKLASS: type = 1; break; default: ShouldNotReachHere(); } report(type)

The patch changes the following functions and defines to never return: - fatal(...) - ShouldNotCallThis() - ShouldNotReachHere() - reportvmoutofmemory(...) You changed the behaviour of this one in the Debugging case - it no longer returns. - vmexitoutofmemory(...) but leaves the following unchanged: - Unimplemented() - Untested(...) - guarantee(...) 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. The approach seems inconsistent though - sometimes a switch to a guarantee, sometimes removal of the destructor and making it private (which doesn't quite give the same level of protection). http://cr.openjdk.java.net/~stefank/8065585/webrev.01/ https://bugs.openjdk.java.net/browse/JDK-8065585 In summary: The BREAKPOINTs have been moved out of the macros into the functions, and the macros then simplified to direct calls. Ok. Some calls now have the NORETURNATTRIBUTE applied. Ok. Calls to functions that have NORETURNATTRIBUTE can be cleaned up if they previously allowed for the function returning. Ok. So first question: is this attribute handled correctly by all the compilers we need to consider? Second, more important question: have you examined how this attribute affects the ability to walk the stack? We have already seen issues on some platforms where library functions, like abort(), have the noreturn attribute and as a result the call is optimized in a way that prevents the stack from being walked - see eg: https://git.matricom.net/Firmware/bionic/commit/5f32207a3db0bea3ca1c7f4b2b563c11b895f276 though this: https://www.raspberrypi.org/forums/viewtopic.php?t=60540&p=451729 suggests that problem may have been addressed by the libc folk. But it still raises the question as to how our own noreturn functions will be handled and how they will affect stacktrace generation in hserr logs or via gdb.

I added a call to fatal(...) in the GC code. I get correct stacktraces in gdb, but the stacktraces in the hs_err files are broken with fastdebug and product builds:

Stack: [0x00007f12518d2000,0x00007f12519d3000], sp=0x00007f12519d0eb0,
free space=1019k Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code) V [libjvm.so+0x11db44a] VMError::report_and_die()+0x1ba V [libjvm.so+0x7efb80] report_vm_error(char const*, int, char const*, char const*)+0x90 V [libjvm.so+0x7efc49] report_vm_error_noreturn(char const*, int, char const*, char const*)+0x9 V [libjvm.so+0x7efc63] V [libjvm.so+0xfd7937] V [libjvm.so+0xfeec51] ...

Thanks, StefanK

Thanks, David

Thanks, StefanK

More information about the hotspot-dev mailing list