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

[Thomas Stüfe]

Hi David,

thank you for fixing this!

I just wondered why we never hit this on MacOS, then I remembered that we
generate SIGILL instead to test secondary signal handling, and that was not
accepted by you guys because generating SIGILL needed processor specific
hacks.

The new solution is cleaner and swapping raise() with pthread_kill() does
not make the source any more platform dependend than it already is.

Kind Regards, Thomas


On Thu, Nov 12, 2015 at 7:33 AM, David Holmes <david.holmes at oracle.com>
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
>