RFR(s): 8143291: Remove redundant coding around os::exception_name
Thomas Stüfe
thomas.stuefe at gmail.com
Thu Nov 26 09:30:25 UTC 2015
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.
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.
Kind Regards, Thomas
On Thu, Nov 26, 2015 at 7:35 AM, David Holmes <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>> 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