[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(s): 8143291: Remove redundant coding around os::exception_name
From:       Thomas_Stüfe <thomas.stuefe () gmail ! com>
Date:       2015-11-30 8:11:19
Message-ID: CAA-vtUzzDhBVav838QCNhSDUvW0=e+JohYvTU1X=ARu5f8sAGA () mail ! gmail ! com
[Download RAW message or body]

Hi Coleen, David,

new webrev here:

http://cr.openjdk.java.net/~stuefe/webrevs/8143291/webrev.04/webrev/

The only change are the deleted bogus comments in jvm_solaris.cpp Coleen
did ask for.

Kind Regards, Thaoms

On Mon, Nov 30, 2015 at 5:27 AM, David Holmes <david.holmes@oracle.com>
wrote:

> On 29/11/2015 3:33 AM, Coleen Phillimore wrote:
>
>>
>> I am on US Thanksgiving holiday but this looks great.  I'm happy to see
>> these cleanups.  It'll make working on the code better.
>>
>> Can you remove the bogus
>>
>>   //Reconciliation History
>>
>>
>> lines from jvm_solaris.cpp while you're here?
>>
>
> I can do that before pushing unless Thomas gets to it first. Due to
> internal constraints I have to wait until after Monday to push anything a=
t
> the moment.
>
> Are JVM_RaiseSignal and JVM_RegisterSignal on xnix platforms mostly
>> duplicate now?  A further RFE?
>>
>
> Yes a further RFE. A lot of the signal related code could be simplified
> and refactored I think.
>
> Thanks,
> David
>
>
> Thanks,
>> Coleen
>>
>> On 11/27/15 11:04 AM, Thomas St=C3=BCfe wrote:
>>
>>> Ping...
>>>
>>> Could I have a second review please?
>>>
>>> The current webrev is:
>>> http://cr.openjdk.java.net/~stuefe/webrevs/8143291/webrev.03/webrev/
>>> And the bug report:    https://bugs.openjdk.java.net/browse/JDK-8143291
>>>
>>> Thank you!
>>>
>>> ..Thomas
>>>
>>>
>>> On Thu, Nov 19, 2015 at 11:23 AM, Thomas St=C3=BCfe <thomas.stuefe@gmai=
l.com>
>>> 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 inver=
se
>>>> 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.
>>>> I am not sure whether I am overcautious here - should I treat the othe=
r
>>>> 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 retu=
rn
>>>> 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
>>>>
>>>>
>>>>
>>>>
>>
[prev in list] [next in list] [prev in thread] [next in thread]