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

[David Holmes]

On 18/11/2015 11:22 PM, Thomas Stüfe wrote:
> Hi Dmitry, David,
>
>
> On Wed, Nov 18, 2015 at 1:28 PM, Dmitry Samersoff
> <dmitry.samersoff at oracle.com <mailto: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.

Right so we prefer to get a real FPE and only use pthread_kill as a 
fallback.

David

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