RFR: 8065585: Change ShouldNotReachHere() to never return

[Stefan Karlsson]

On 2015-04-15 16:36, Per Liden wrote:
> 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!

Thanks!

StefanK

>
> /Per