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

[David Holmes]

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
>
>