RFR: 8254824: SignalHandlerMark have no purpose

[Thomas Stüfe]

Hi Robin,

Thanks for clarifying! My mail was partly to check if I had overlooked
something.

I will, as part of the cleanup, move some of the more obvious common
sections out of the platform signal handlers up to the shared wrapper
function. I will do this for the crash protector too. And then tweak the
comment somewhat to my liking :)

Thanks, Thomas


On Fri, Oct 30, 2020, 09:28 Robbin Ehn <robbin.ehn at oracle.com> wrote:

> Hi Thomas,
>
> The comment was specificity just for the signalhandler scope.
> I assumed that any one changing the signal handler would know what
> "longjmp away" means any epilogue code will not run.
>
> Since a RAII destructor is much more likely to get missed, I agreed with
> David that a reminder of RAII destructors could be useful.
>
> The user of ThreadCrashProtection::call() must take much more
> precaution, very few pieces of code can be called reliable with it.
>
> > IIUC it should not say "don't use RAII" but "in anything you do in all
> > frames between os::ThreadCrashProtection::call() and
> > os::ThreadCrashProtection::check_crash_protection(), don't rely on any
> > epilogue code".
>
> So you have this comment in the declaration:
> /*
>   * Crash protection for the JfrSampler thread. Wrap the callback
>   * with a sigsetjmp and in case of a SIGSEGV/SIGBUS we siglongjmp
>   * back.
>   * To be able to use this - don't take locks, don't rely on
> destructors,
>   * don't make OS library calls, don't allocate memory, don't print,
>   * don't call code that could leave the heap / memory in an
> inconsistent state,
>   * or anything else where we are not in control if we suddenly jump out.
>   */
> class ThreadCrashProtection : public StackObj {
>
>
> https://github.com/openjdk/jdk/blob/379ba80eb7999f60fb12a08a9d0b2ff16263ab23/src/hotspot/os/posix/os_posix.hpp#L115
>
> If you think you can improve the comments feel free to improve!
>
> Thanks, Robbin
>
> >
> > Thanks, Thomas
> >
> >     -------------
> >
> >     PR: https://git.openjdk.java.net/jdk/pull/677
> >     <https://git.openjdk.java.net/jdk/pull/677>
> >
>