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

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 oswindows.cpp. I moved the name-to-number-array into os::getsignalnumber() 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::isvalidsignal(), 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 osposix.cpp, it could be used at various places to verify signal numbers. For instance, in the validation of input for JAVASRSIGNUM. 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 oswindows.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::getsignalnumber() and moved JVMFindSignal to jvm.cpp, removing the different implementations from jvm.cpp. Thanks - testing it now. I still am not totally happy with adding os::getsignalnumber() to windows. I feel it is a bit confusing: - we have os::getsignalnumber() 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::exceptionname(), which under windows returns the name of a given machine exception (e.g. "EXCEPTIONACCESSVIOLATION") and under Posix a signal name. Yes perhaps "exception" should have been something more platform neutral. Not sure what though. - we have os::Posix::getsignalname(), 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::isvalidsignal. 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 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 getsignalnumber/name up into os:: seems kind of a stretch. But we already have a bunch of os:: level signal functions: static void signalinit(); static void signalinitpd(); static void signalnotify(int signalnumber); static void* signal(int signalnumber, void* handler); static void signalraise(int signalnumber); static int signalwait(); static int signallookup(); 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 jvmposix.cpp, moving JVMFindSignal() from all jvm<linux|bsd|solaris|aix>.cpp to jvmposix.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::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