RFR: 8065585: Change ShouldNotReachHere() to never return

[Jon Masamitsu]

On 4/15/2015 11:52 AM, Stefan Karlsson wrote:
> On 2015-04-15 20:16, Jon Masamitsu wrote:
>> http://cr.openjdk.java.net/~stefank/8065585/webrev.02/src/share/vm/utilities/debug.cpp.frames.html 
>>
>>
>>>   210 // A function that never returns. Tells the compilers
>>>   211 // that the control flow stops at the call of this function.
>>>   212 ATTRIBUTE_NORETURN static void noreturn_function() {
>>>   213   while (true) {
>>>   214     os::naked_short_sleep(10);
>>>   215   }
>>>   216 }
>>
>> Did you consider using just os::infinite_sleep()
>> inside of noreturn_function()?   It doesn't do a
>> short sleep but I don't know what using the
>> short sleep vs. a longer sleep is meant to do.
>
> I did consider it once, but rejected it for some reason that I can't 
> remember at this point. I could probably get rid of the 
> noreturn_function(), tag infinit_sleep with ATTRIBUTE_NORETURN, and 
> use it instead.

 From the code readability infinite_sleep() certainly tells you what 
it's doing.  I also like it
for the longer sleep times.   I recall several times looking at my 
machine and wondering
what was eating up all the cpu.  It turned out to be tests that had 
failed (with fastdebug
JVMs) that were spinning in a tight wait loops.  It stopped happening 
shortly after I first
noticed it so I didn't look into what had changed.

Jon
>
> Thanks,
> StefanK
>
>>
>> Not done yet.
>>
>> Jon
>>
>>
>> On 4/15/2015 3:49 AM, 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.
>>>
>>> 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
>>>
>>> Thanks,
>>> StefanK
>>
>