RFR: 8065585: Change ShouldNotReachHere() to never return

[Stefan Karlsson]

On 2015-04-15 14:09, Per Liden wrote:
> Hi Stefan,
>
> Nice cleanup!

Thanks, Per.

> Some comments below.
>
> On 2015-04-15 12:49, 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(...)
>> - 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.
>
> guarantee() should not have this attribute as it should be able to 
> return.

Of course.

>
>>
>> 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
>
> Looks good, just a few minor suggestions:
>
> - NORETURN_ATTRIBUTE -> ATTRIBUTE_NORETURN, to better match the 
> existing ATTRIBUTE_PRINTF?

I've gone with your suggestion to rename the define and get rid of the 
macro function parameter.


>
> - Make noreturn_function() static and move it into debug.cpp?

Done.

http://cr.openjdk.java.net/~stefank/8065585/webrev.02.delta/
http://cr.openjdk.java.net/~stefank/8065585/webrev.02/

Thanks,
StefanK
>
> cheers,
> /Per