8065585: Change ShouldNotReachHere() to never return

[Stefan Karlsson]

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 is_marked() {
>>     // 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 TYPE_OOP: type = 0; break;
>>     case TYPE_KLASS: type = 1; break;
>>     default: ShouldNotReachHere();
>> }
>> report(type)
>>
>>
>> The patch changes the following functions and defines to never return:
>> - fatal(...)
>> - ShouldNotCallThis()
>> - ShouldNotReachHere()
>> - report_vm_out_of_memory(...)
>
> You changed the behaviour of this one in the Debugging case - it no 
> longer returns.
>
>> - vm_exit_out_of_memory(...)
>>
>> 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 NORETURN_ATTRIBUTE applied. Ok.
>
> Calls to functions that have NORETURN_ATTRIBUTE 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 hs_err 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
>>