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

[David Holmes]

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.

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