RFR(s): 8143291: Remove redundant coding around os::exception_name
David Holmes
david.holmes at oracle.com
Thu Nov 26 10:38:42 UTC 2015
On 26/11/2015 7:30 PM, Thomas Stüfe wrote:
> Hi David,
>
> here the new webrev:
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8143291/webrev.03/webrev/
>
> Only changes are in os_windows.cpp. I moved the name-to-number-array
> into os::get_signal_number() and made it anonymous.
Okay - though you could have just deleted the duplicate definition.
If we can get a second reviewer I will push this for you. Though with
the ThanksGiving holiday that might be a problem :)
> As for os::Posix::is_valid_signal(), I think that coding was originally
> from me :) but I find it actually a useful to the Posix-related toolset.
> Instead of hiding it in os_posix.cpp, it could be used at various places
> to verify signal numbers. For instance, in the validation of input for
> _JAVA_SR_SIGNUM.
Maybe. Meanwhile they are just exposed but unused (externally) API.
Cheers,
David
>
> Kind Regards, Thomas
>
>
> On Thu, Nov 26, 2015 at 7:35 AM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
> Hi Thomas,
>
> In os_windows.cpp you added:
>
> + struct siglabel {
> + char *name;
> + int number;
> + };
>
> but that struct is already defined further up in the file.
>
> Otherwise all seems okay.
>
> Thanks,
> David
>
>
> On 26/11/2015 2:16 PM, David Holmes wrote:
>
> 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>
> <mailto: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>>
> <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,
>
> 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>>>
> <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
> <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