RFR: JDK-8255711: Fix and unify hotspot signal handlers

[David Holmes]

<trimming>

On 4/11/2020 4:34 pm, Thomas Stüfe wrote:
>>     On the documentation front we could at least explain what the flag
>>     really does. Rather than saying:
>> 
>>        product(bool, AllowUserSignalHandlers, false,
>>          \
>>                 "Do not complain if the application installs signal
>>     handlers
>>     "
>> 
>>     we could say something like:
>> 
>>        product(bool, AllowUserSignalHandlers, false,
>>          \
>>                 "Allow the application to install the primary signal
>>     handlers
>>     instead of the JVM." \
>> 
>>     and we (I?) could update the java manpage.
> 
> I can update the text description, but I would like to defer any 
> additional work for this switch to some other RFE.

Sure the manpage update can be another RFE.

>      > 10) When invoking the fatal error handler, we extract the pc from
>     the context and hand it over as "faulting pc". For SIGILL and
>     SIGFPE, this is not totally correct. According to POSIX [3], for
>     those signals the address of the faulting instruction is handed over
>     in `si_info.si_addr`.
>      >
>      > On most platforms this does not matter, they are the same. But on
>     some architectures the pc in the signal context actually points
>     somewhere else, e.g. beyond the faulting instruction. Therefore
>     `si_info.si_addr` is the better choice.
> 
>     The Posix spec also states "For some implementations, the value of
>     si_addr may be inaccurate." - so I'm not at all sure which "pc" we
>     should be trusting here? I thought the ucontext was the detailed
>     platform specific "context" object that we should extract information
>     from. Which architectures give different values in the two and is there
>     some documentation stating what happens for any given os/cpu?
> 
> 
> I overlooked this. Well, this is a helpful standard :)
> 
> I saw this happen on s390 and on pa-risc. Before this patch, I did 
> correct this in the platform handler. Out of caution I could #ifdef this 
> section to s390.

I prefer not see any change in behaviour for platforms where no problem 
has been observed. Also see what seems to be related comment in 
./cpu/arm/vm_version_arm_32.cpp

// JVM_handle_linux_signal moves PC here if SIGILL happens

>      > ----
>      >
>      > The changes in this patch:
>      >
>      > a) hotspot signal handling is now done by the following functions:
>      >
>      > <OS>                 <foreign code, probably signal handler too>
>      >    |                           |
>      >    v                           v
>      >   javaSignalHandler       JVM_handle_linux_signal()
>      >         |                   /
>      >         v                  v
>      >       javaSignalHandler_inner
> 
>     Not clear why we need the _inner version. Why can't we just have
>     javaSignalHandler which is installed as the handler and which is called
>     by JVM_handle_XXX_signal?
> 
> 
> Because JVM_handle_XXX_signal has one more argument than the standard 
> signal handler (abort_if_unrecognized).

Okay so why introduce this shape instead of keeping the existing form:

javaSignalHandler -> JVM_handle_xxx_signal(..., true) -> 
javaSignalHandler_inner

? With the new arrangement the equivalence between javaSignalHandler and 
JVM_handle_xxx_signal can only be seen by inspecting the code of both.

Thanks,
David