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

[David Holmes]

Hi Thomas,

On 25/11/2015 12:40 AM, Thomas Stüfe wrote:
> Hi David,
>
> On Mon, Nov 23, 2015 at 7:26 AM, David Holmes <david.holmes at oracle.com
> <mailto: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.

But we already have a bunch of os:: level signal functions:

   static void  signal_init();
   static void  signal_init_pd();
   static void  signal_notify(int signal_number);
   static void* signal(int signal_number, void* handler);
   static void  signal_raise(int signal_number);
   static int   signal_wait();
   static int   signal_lookup();

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

This won't change any actual code execution on Windows, simply move the 
existing logic from one place to another.

Cheers,
David
-----

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