RFR: 8065585: Change ShouldNotReachHere() to never return

[Per Liden]

Hi Stefan,

On 2015-04-15 16:30, Stefan Karlsson wrote:
> 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/

Looks good!

/Per