(S) RFR: 8139300: Internal Error (vm/utilities/debug.cpp:399), # Error: ShouldNotReachHere()

[Dmitry Samersoff]

David,

Behavior of FPE is slightly different from one of pthread_kill so it
might be better to always use pthread_kill and put 331,332 under #ifdef
WINDOWS.

But it's just an opinion - I'm OK with either solution.

-Dmitry

On 2015-11-18 06:56, David Holmes wrote:
> On 17/11/2015 6:41 PM, Dmitry Samersoff wrote:
>> David,
>>
>> Do we still need:
>>
>> 331   volatile int x = 0;
>> 332   volatile int y = 1/x;
> 
> That bit is still required for windows, and probably works just fine
> everywhere except OSX.
> 
>> I guess current compiler tendency is to warn/optimize as much as
>> possible, so sooner or later we will have a warning/NOP here on all
>> platforms.
> 
> The current fix addresses all platforms but Windows. So only a future
> fix for Windows might be needed.
> 
> David
> 
>> -Dmitry
>>
>>
>> On 2015-11-12 09:33, David Holmes wrote:
>>> webrev: http://cr.openjdk.java.net/~dholmes/8139300/webrev/
>>>
>>> Bug report is not public sorry.
>>>
>>> In the crash handler test logic in the VM we have:
>>>
>>> static void crash_with_sigfpe() {
>>>     // generate a native synchronous SIGFPE where possible;
>>>     // if that did not cause a signal (e.g. on ppc), just
>>>     // raise the signal.
>>>     volatile int x = 0;
>>>     volatile int y = 1/x;
>>>   #ifndef _WIN32
>>>     raise(SIGFPE);
>>>   #endif
>>>   } // end: crash_with_sigfpe
>>>
>>> and it is used here:
>>>
>>>   402     case 15: crash_with_sigfpe(); break;
>>>   403
>>>   404     default: tty->print_cr("ERROR: %d: unexpected test_num
>>> value.",
>>> how);
>>>   405   }
>>>   406   ShouldNotReachHere();
>>>
>>> We recently updated the compiler on OSX and started seeing occasional
>>> failures of the test exercising this code, the failure mode being that
>>> we hit the ShouldNotReachHere() at line #406.
>>>
>>> It seems the new compiler may not be generating code for the "y=1/x;"**
>>> so we don't get the SIGFPE and so proceed to raise it directly. That
>>> then hits an apparent bug on OSX where raise always sends the signal to
>>> the main thread not the current thread as required by POSIX in a
>>> multi-threaded app. Consequently raise() could return and we'd hit the
>>> ShouldNotReachHere().
>>>
>>> Whether the test failed or appeared to pass depended on which thread got
>>> into the error handler first (though I'm still unclear on the finer
>>> details of that potential interaction - the other thread should have
>>> seen error reporting was already in progress by the recursively failing
>>> thread!)
>>>
>>> The solution I chose is to simply convert raise(sig) into its POSIX
>>> specified equivalent: pthread_kill(pthreead_self(), sig);
>>>
>>> I'm a little reluctant having pthread functions in a shared file, but
>>> AFAIK all our platforms supports pthreads. This also doesn't seem any
>>> worse than assuming raise() exists on all platforms. But I could add
>>> some ifdef _POSIX_C_SOURCE if there is concern (and even have a #error
>>> for unknown platforms) ?
>>>
>>> ** I'm not surprised a compiler might elide the attempted division by
>>> zero. I don't think volatile on a local has any meaning and so could
>>> easily be optimized out completely. I overlooked that in the original
>>> commit of this logic, and it worked so ...
>>>
>>> Thanks,
>>> David
>>
>>


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.