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

[Thomas Stüfe]

Hi Dmitry, David,


On Wed, Nov 18, 2015 at 1:28 PM, Dmitry Samersoff <
dmitry.samersoff at oracle.com> wrote:

> 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.
>
>
This is true. With a real SIGFPE you get a real context with a real crash
pc. That was the reason we went through so much trouble generating real
signals - because we wanted to test that the pc in the hs-err file is
displayed correctly.

Regards, Thomas



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