RFR(s): 8143291: Remove redundant coding around os::exception_name
Thomas Stüfe
thomas.stuefe at gmail.com
Fri Nov 27 09:14:10 UTC 2015
Thanks David! :)
On Thu, Nov 26, 2015 at 11:38 AM, David Holmes <david.holmes at oracle.com>
wrote:
> 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