> On Apr 27, 2018, at 7:25 PM, David Holmes <david.hol...@oracle.com> wrote: > > We discussed this offline and Gary pointed out that, at least in the VMError > case the attempt to SEGV by dereferencing zero is one of a specific number of > crash inducing cases, others of which include trying to trigger SEGV at > non-zero address and explicit signal raising. So changing the code to raise > the signal directly is not really appropriate - and the code in VMError knows > the attempt may not result in a crash. > > So I am okay with just disabling the compilation warnings for these two cases.
I feel pretty uncomfortable with code that is intentionally invoking undefined behavior and expecting anything at all useful to come from that. I think in the frame::oops_do_internal case, the code used when CrashGCForDumpingJavaThread should use os::signal_raise, as suggested by David. Except there is the problem that David diagnosed in JDK-8139300, that OSX raise sends a signal to the main thread instead of the current thread. I wonder if that is still true with our new minimum supported OSX version? Man pages on my Mac say raise sends the signal to the current thread. (And I'm surprised that bug was fixed that way, rather than using os::signal_raise and making sure that worked properly.) In VMError::controlled_crash, not only is there no guarantee the write will crash, there's also no guarantee the break will do anything either. Since executing the write clearly invokes UB, the subsequent break can be assumed to be unreachable, or demons may fly out your nose. But since both of these are !PRODUCT, I guess I'm okay with suppressing the warning for now, as a way to move forward with the compiler upgrade. However, I'd like the scope of that warning suppression narrowed if possible, such as by moving it down to inside the #ifndef PRODUCT protecting the test code. We don't need the suppression for the entire file. For the frame case, does Solaris support any sort of push/pop diagnostic control? And it looks like there is further cleanup needed in these areas.
