RFR(s): 8143291: Remove redundant coding around os::exception_name
David Holmes
david.holmes at oracle.com
Thu Nov 26 04:16:38 UTC 2015
Hi Thomas,
On 25/11/2015 11:29 PM, Thomas Stüfe wrote:
> Hi David,
>
> new webrev:
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8143291/webrev.02/webrev/index.html
>
> Changes:
> 1) added SIGSTKFLT
> 2) as per your suggestion, added os::get_signal_number() and moved
> JVM_FindSignal to jvm.cpp, removing the different implementations from
> jvm_<os>.cpp.
Thanks - testing it now.
> I still am not totally happy with adding os::get_signal_number() to
> windows. I feel it is a bit confusing:
> - we have os::get_signal_number() which takes Posix signal names (even
> on Windows, even though signal support is almost nonexistent)
The Windows version only recognizes the subset of those names that
Windows also defines in signal.h. So I really don't see any issue here.
> - we have os::exception_name(), which under windows returns the name of
> a given machine exception (e.g. "EXCEPTION_ACCESS_VIOLATION") and under
> Posix a signal name.
Yes perhaps "exception" should have been something more platform
neutral. Not sure what though.
> - we have os::Posix::get_signal_name(), which only exists on Posix.
Yeah I don't see any reason for that to be defined in the API at all -
it could just be a file-static helper function. Ditto for
os::Posix::is_valid_signal.
> But cleaning this up goes a bit beyond what I planned to so with my fix.
Appreciate what you have done!
Thanks,
David
> Kind Regards, Thomas
>
>
>
> On Wed, Nov 25, 2015 at 10:13 AM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
> 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>
> <mailto: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>>
> <mailto: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
>
>
>
>
More information about the hotspot-runtime-dev
mailing list