RFR(s): 8143291: Remove redundant coding around os::exception_name

[Thomas Stüfe]

Hi David,

On Mon, Nov 23, 2015 at 7:26 AM, David Holmes <david.holmes at oracle.com>
wrote:

> Hi Thomas,
>
> Looks good!
>
> On 20/11/2015 8:14 PM, Thomas Stüfe wrote:
>
>> Hi David,
>>
>> here my second webrev:
>>
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8143291/webrev.01/webrev/index.html
>>
>
> os_posix.cpp doesn't define SIGSTKFLT (currently defined for Linux and AIX)
>
> I think we can we now move:
>
>  JVM_ENTRY_NO_ENV(jint, JVM_FindSignal(const char *name))
>    return os::Posix::get_signal_number(name);
>  JVM_END
>
> into jvm.cpp (from jvm_<os>.cpp) - that would clean things up a bit more.
> We can promote  os::Posix::get_signal_number to be an os:: function (all
> platforms have signals, even windows) called from the shared JVM_FindSignal
> - then create a Windows version in os_windows.cpp by moving the code
> currently in jvm_windows.cpp JVM_FindSignal.
>
>
I don't know... moving get_signal_number/name up into os:: seems kind of a
stretch. I would prefer to leave windows out of it for now because I do not
understand how windows signal handling (is there really any?) corresponds
to Structured Exception Handling.

How about: adding a jvm_posix.cpp, moving JVM_FindSignal() from all
jvm_<linux|bsd|solaris|aix>.cpp to jvm_posix.cpp? That would be almost as
good...

Kind Regards, Thomas



> Thanks,
> David
> -----
>
>
> Remarks below.
>>
>> On Thu, Nov 19, 2015 at 12:56 PM, David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.com>> wrote:
>>
>>     Hi Thomas,
>>
>>     I like the idea of this, but it will take me a little time to check
>>     all the details. More below ...
>>
>>     On 19/11/2015 8:23 PM, Thomas Stüfe wrote:
>>
>>         Hi,
>>
>>         please take a look at this change.
>>
>>         bug: https://bugs.openjdk.java.net/browse/JDK-8143291
>>         webrev:
>>
>> http://cr.openjdk.java.net/~stuefe/webrevs/8143291/webrev.00/webrev/
>>
>>         This fix does some cleanups around os::exception_name().
>>
>>         - os::exception_name() is identical on all Posix platforms (it
>>         returns a
>>         signal name string for a signal number), so it can be merged into
>>         os_posix.cpp
>>
>>         - There is no need for a platform-specific implementation, as we
>>         have
>>         already os::Posix::get_signal_name(). Use that instead of
>>         platform-specific
>>         solutions.
>>
>>         - I added a function os::Posix::get_signal_number() which is the
>>         inverse of
>>         os::Posix::get_signal_name().
>>
>>         - Before, a signal-to-number table was kept in jvm_<os>.cpp.
>>         That was used
>>         to implement os::exception_name() and also for JVM_FindSignal
>>         -> on AIX, I removed the coding altogether and used
>>         os::Posix::get_signal_number() as a base for JVM_FindSignal.
>>         -> on the other Unices, I did not feel so confident, because
>>         strictly
>>         spoken we may change the behaviour slightly to before:
>>         os::Posix::get_signal_name() knows more signal names than the
>>         platform
>>         specific tables knew before, so now Signal.findSignal("<name>")
>>         would
>>         return more matches than before.
>>
>>
>>     How so? If the additional names are platform specific then they
>>     won't exist at build time on the other platforms and so will be
>>     elided from the table.
>>
>>
>> I agree and changed jvm_bsd.cpp, jvm_linux.cpp and jvm_solaris.cpp to
>> use os::Posix::get_signal_number for JVM_findSignal. That removes more
>> duplicate code.
>>
>>     BTW the existing Solaris code is incorrect when we start building on
>>     Solaris 11, so it would be good to use something that uses the
>>     actual build time signal value rather than the current array based
>>     approach.
>>
>>
>> Nice, that should be automatically fixed too then.
>>
>> Kind Regards, Thomas
>>
>>     Thanks,
>>     David
>>
>>
>>         I am not sure whether I am overcautious here - should I treat
>>         the other
>>         Unices the same way I treated AIX, i.e. implement
>>         Signal.findSignal("<name>") -> JVM_FindSignal via
>>         os::Posix::get_signal_number()? This would further simplify the
>>         coding.
>>
>>         Oh, this fix also fixes an issue where os::exception_name()
>>         would return
>>         NULL for an unknown signal number, but no caller ever checks for
>>         NULL
>>         before printing the result. The new os::exception_name() always
>>         returns a
>>         string and also distinguishes between "unknown but valid signal"
>> and
>>         "invalid signal".
>>
>>         Kind Regards, Thomas
>>
>>
>>