8065585: Change ShouldNotReachHere() to never return

[Stefan Karlsson]

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.

Yes. And I changed the behavior for all the others as well. We can't 
allow Debugging to let us return from the functions if we are going to 
do this change.

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

Some times you can't remove the destructor. For example, the 
g1StringDedupThread code has to provide a destructor, or the linker will 
complain about a missing vtable entry.

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

It works for the Linux and Windows platforms that are run in JPRT. The 
Solaris Studio documentation describes it, and it passes JPRT, but I 
don't remember if the equivalent to -Wreturn-type is on when we compile 
on Solaris.

>
> 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 can do experiments to see if we still can walk the stack.

Thanks,
StefanK

>
> Thanks,
> David
>
>>
>> Thanks,
>> StefanK
>>