RFR(s): 8143291: Remove redundant coding around os::exception_name (original) (raw)

Thomas Stüfe thomas.stuefe at gmail.com
Tue Nov 24 14:40:38 UTC 2015

Hi David,

On Mon, Nov 23, 2015 at 7:26 AM, David Holmes <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 osposix.cpp doesn't define SIGSTKFLT (currently defined for Linux and AIX) I think we can we now move: JVMENTRYNOENV(jint, JVMFindSignal(const char *name)) return os::Posix::getsignalnumber(name); JVMEND into jvm.cpp (from jvm.cpp) - that would clean things up a bit more. We can promote os::Posix::getsignalnumber to be an os:: function (all platforms have signals, even windows) called from the shared JVMFindSignal - then create a Windows version in oswindows.cpp by moving the code currently in jvmwindows.cpp JVMFindSignal. I don't know... moving get_signal_number/name up into os:: seems kind of a stretch. 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.

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>> 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::exceptionname(). - os::exceptionname() is identical on all Posix platforms (it returns a signal name string for a signal number), so it can be merged into osposix.cpp - There is no need for a platform-specific implementation, as we have already os::Posix::getsignalname(). Use that instead of platform-specific solutions. - I added a function os::Posix::getsignalnumber() which is the inverse of os::Posix::getsignalname(). - Before, a signal-to-number table was kept in jvm.cpp. That was used to implement os::exceptionname() and also for JVMFindSignal -> on AIX, I removed the coding altogether and used os::Posix::getsignalnumber() as a base for JVMFindSignal. -> on the other Unices, I did not feel so confident, because strictly spoken we may change the behaviour slightly to before: os::Posix::getsignalname() knows more signal names than the platform specific tables knew before, so now Signal.findSignal("") 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 jvmbsd.cpp, jvmlinux.cpp and jvmsolaris.cpp to use os::Posix::getsignalnumber for JVMfindSignal. 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("") -> JVMFindSignal via os::Posix::getsignalnumber()? This would further simplify the coding. Oh, this fix also fixes an issue where os::exceptionname() would return NULL for an unknown signal number, but no caller ever checks for NULL before printing the result. The new os::exceptionname() 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