RFR: 8136978 Much nearly duplicated code for vmError support (original) (raw)

Thomas Stüfe thomas.stuefe at gmail.com
Wed Nov 25 10:48:42 UTC 2015

Hi David,

On Wed, Nov 25, 2015 at 10:45 AM, David Holmes <david.holmes at oracle.com> wrote:

On 25/11/2015 5:02 PM, Thomas Stüfe wrote:

Hi Sebastian,

assuming the build error fix for AIX is the only change for webrev 07, I'm fine with it. The question about thrsetsigmask vs pthreadsigmask on Solaris remains and should be answered by a Solaris expert IMHO. I have answered this a number of times already (though not claiming the Solaris expert label :) ). See for example: https://docs.oracle.com/cd/E19455-01/806-0630/6j9vkb8hh/index.html "As POSIX threads and Solaris threads are fully compatible even within the same process ..." Use of pthreadsigmask is fine. We tried to convert from thr to pthread APIs a few years back but the job got unmanageable (due to issues with T1 and T2 thread libraries) and had to be dropped. thrcreate allows a thread to be started in the suspended state, but otherwise a simple replacement would work. Such a change is worth looking at again now we have cleaned out a lot of the ancient crud eg T1 stuff. Thanks, David Thank you for clarifying!

...Thomas

Otherwise, thank you for this cleanup!

..Thomas On Tue, Nov 24, 2015 at 5:54 PM, Sebastian Sickelmann <sebastian.sickelmann at gmx.de <mailto:sebastian.sickelmann at gmx.de>> wrote: Hi, sorry for the typo. The new webrev is here: http://cr.openjdk.java.net/~sebastian/8136978/webrev.07 I will also execute the mentioned tests on linux. -- Sebastian

On 11/24/2015 05:09 PM, Thomas Stüfe wrote: Hi all,

http://cr.openjdk.java.net/~sebastian/8136978/webrev.06 <http://cr.openjdk.java.net/%7Esebastian/8136978/webrev.06> 1) build error, there is a typo in osaix.cpp (void bool startdebugging()), please remove the "void" 2) os::Posix::unblockthreadsignalmask() now uses pthreadsigmask for all Unices. Are we sure pthreadsigmask works with solaris threads? I am not a Solaris expert, but I always thought solaris threads and posix threads are different things. We seem to take care to use consistently the "thrxx" APIs on Solaris. Won't that be a problem? In the end, a good test would be to execute runtime/ErrorHandling/SafeFetchInErrorHandlingTest.java jtreg test, because it tests the secondary signal handling. I will run this test on AIX once the build is through. Kind Regards, Thomas On Mon, Nov 23, 2015 at 4:41 AM, David Holmes <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote: On 21/11/2015 2:38 AM, Coleen Phillimore wrote: On 11/20/15 10:17 AM, Thomas Stüfe wrote: Hi David, On Fri, Nov 20, 2015 at 1:44 PM, David Holmes <<mailto:david.holmes at oracle.com>david.holmes at oracle.com <mailto:david.holmes at oracle.com> <mailto:david.holmes at oracle.com_ _<mailto:david.holmes at oracle.com>>> wrote: On 20/11/2015 10:22 PM, Sebastian Sickelmann wrote: Hi, i created a new webrev with the suggested changes: Sorry Sebastian but I remain totally opposed to the os::Posix::ucontextgetpc related changes. Moving them out of the namespace for the os specific classes makes no sense to me - the Posix class is for shared implementations and these have to be platform specific. I understand that, but I would have liked a version of ucontextgetpc/ucontextsetpc outside an os-specific namespace: Now you have to use os::Aix::ucontextgetpc(), os::Linux::ucontextgetpc() etc. I would love to get rid of that nested platform dependend name scope and have a generic name for all, be it os::Posix::uncontextgetpc or just os::ucontextgetpc(). In my mind os::Posix::ucontextgetpc() makes more sense, because there is no windows version for this, and ucontextt is Posix... but I don't have strong emotions. Okay I see now that from an API perspective this is an os::Posix function. The issue is the implementation location. Ideally this would be factored out in a osposix.cpp file in a suitable directory - but that is a lot of messing around for an isolated case. So ... I had this same thought looking at the code, but didn't have a better solution for it. Maybe we could have the call be os::Posix::ucontextgetpc() be in osposix.hpp and have ifdefs to go to os::Linux/Solaris/etc/::ucontextgetpc? Like: static address ucontextgetpc(ucontextt* ctx) { #if TARGETOSFAMILYsolaris return Solaris::ucontextgetpc(ctx); #else ... #endif } Maybe this would have to be in the .cpp file. This would reduce the size of the changeset and you can keep os::Solaris::ucontextgetpc(). This seems like a reasonable compromise to me. The API is in the Posix namespace, the implementation remains in the os.cpp file. The forwarding function in osposix.cpp it a tolerable "evil". Thanks, David Thanks, Coleen Kind Regards, Thomas Solaris can use pthreadsigmask so no need for special casing. Thanks, David http://cr.openjdk.java.net/~sebastian/8136978/webrev.04 <http://cr.openjdk.java.net/%7Esebastian/8136978/webrev.04> <_ _http://cr.openjdk.java.net/%7Esebastian/8136978/webrev.04> I moved the unblocking of signals to osposix.cpp and renamed it to unblockthreadsignalmask The use of sigthreadmask is now replaced with pthreadsigmask. For the remaining sigthreadmask in the codebase I created JDK-8143395. I also removed formatdebugmessage and refactored the content into startdebugging. Can some tell me why this "do {} while (true)" in VMError::showmessagebox is needed which i copied from the original "os-dependent" implementations? Sorry, I really do not understand it. -- Sebastian On 11/20/2015 10:49 AM, David Holmes wrote: On 20/11/2015 7:26 PM, Thomas Stüfe wrote: Hi David, On Fri, Nov 20, 2015 at 8:47 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, On 20/11/2015 5:36 PM, Thomas Stüfe wrote: Hi David, On Fri, Nov 20, 2015 at 6:05 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>>> <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 Sebastian, On 20/11/2015 12:50 AM, Coleen Phillimore wrote: JPRT is our build and test system that runs all the platforms we support. It runs a subset of our tests. It's how we integrate code so that no code that doesn't build and pass tests can get integrated. I think you mean 03. http://cr.openjdk.java.net/~sebastian/8136978/webrev.03/index.html <_ _http://cr.openjdk.java.net/%7Esebastian/8136978/webrev.03/index.html> <_ _http://cr.openjdk.java.net/%7Esebastian/8136978/webrev.03/index.html> I'm a little confused. I would expect anything in the os::Posix namespace to be defined in the os/posix/vm/osposix.cpp file as it is supposed to be a shared implementation. Otherwise it should just be in the os:: namespace and have an os.cpp implementation - as you have here. I think this is a borderline case. ucontextt is Posix, so it fits into osposix.hpp nicely, but the implementation for ucontextget/setpc() is deeply platform dependend and must live in platform specific files. Sorry I was referring to: + int os::Posix::setthreadsignalmaskunblocked(const sigsett *set) { + return pthreadsigmask(SIGUNBLOCK, set, NULL); + } Oh, that. We use different APIs for setting thread signal masks: aix sigthreadmask linux,bsd pthreadsigmask solaristhrsigsetmask Solaris can use pthreadsigmask too. Given what I've been doing with pthreadget/setspecific I was assuming that all platforms were supporting the pthreads API - which of course is what os::Posix represents. Situation on AIX is confusing; there is pthreadsigmask(), but for me it is not clear if this is a simple alias for sigprocmask(), i.e. sets the signal mask for the whole process. IBM offers "sigthreadmask()" instead for multihtreaded programs. But I just found that we use pthreadsigmask() all over the place in osaix.cpp, so this needs checking. Maybe we can simply switch to pthreadsigmask() for AIX. This suggests pthreadsigmask == sigthreadmask: http://www-01.ibm.com/support/knowledgecenter/sswaix53/com.ibm.aix.basetechref/doc/basetrf1/pthreadsigmask.htm%23i08219704tanab Cheers, David ----- For the implementation this means we either live with 2-3 small #ifdef sections in osposix.cpp or we introduce osposix.cpp. I strongly prefer the first variant, but that is a matter of taste. I hadn't noticed the: os::Posix::ucontextgetpc which I assume is more CPU dependent than OS dependent? Both. ucontextt on AIX looks different than on Linux PowerPC. Thats why the implementations live in . We could stretch this a bit by including CONTEXT (the windows variant of uncontextt), typedef a common type for CONTEXT/ucontextt and add implementations for windows too... but I think this is unnecessary. Or, we could put all implementations into osposix.cpp and live with a lot of #ifdefs. Personally, I think Sebastians solution is ok. The whole point of osposix.cpp is to define a common shared implementation, with as few ifdefs as possible. I really do not want to see os::Posix implementations in the os specific files. Need to look at this one more closely. Thanks, David Regards, Thomas ------ Also os::formatdebugmessage looks like a candidate for a shared implementation as well. Agreed here. I even would prefer os::formatdebugmessage() to be folded somehow into os::startdebugging() and be hidden from sight. Kind Regards, Thomas Thanks, David ----- I verified all the code. It looks great! I'm happy to sponsor, pending another code review. Thanks, Coleen On 11/19/15 5:46 AM, Sebastian Sickelmann wrote: Hi Coleen, thanks for finding those errors. I am sorry i missed those. It shows that it is really helpful two test/compile on multiple platforms. What is JPRT? Please find the new webrev here: http://cr.openjdk.java.net/~sebastian/8136978/webrev.02 <http://cr.openjdk.java.net/%7Esebastian/8136978/webrev.02> <_ _http://cr.openjdk.java.net/%7Esebastian/8136978/webrev.02> thanks, Sebastian On 11/19/2015 05:59 AM, Coleen Phillimore wrote: Hi Sebastian, I tested your change through JPRT and there are a few changes I needed to make to get it to pass. One is that I changed os::messagebox to return bool since that's how it's used and the use in vmError.cpp made the Windows compiler complain. Others (hope you can read tell the diffs). There was thrsigsetmask on solaris and a couple places os::formatdebugmessage() you had 'p' rather than 'buf' and one place had 'buflen' rather than 80. Those places os::startdebugging(), it would be better to have sizeof(buf) rather than 80. Otherwise, Reviewed. If you do create a changeset, I will sponsor after another reviewer sees it. thanks, Coleen < ---_ _old/src/os/solaris/vm/ossolaris.cpp_ _2015-11-18 19:05:31.209186804_ _+0100_ _< +++_ _new/src/os/solaris/vm/ossolaris.cpp_ _2015-11-18 19:05:31.129186803_ _+0100_ _--- diff_ _--git_ _a/src/os/solaris/vm/ossolaris.cpp_ _b/src/os/solaris/vm/ossolaris.cpp_ _---_ _a/src/os/solaris/vm/ossolaris.cpp_ _+++_ _b/src/os/solaris/vm/ossolaris.cpp_ _@@_ _-3611,7 +3611,7 @@ void_ _os::printstatistics() { }_ _-int_ _os::messagebox(const char* title,_ _const char* message) {_ _+bool_ _os::messagebox(const char* title,_ _const char* message) {_ _int i;_ _fdStream_ _err(defaultStream::errorfd());_ _for_ _(i = 0; i < 78; i++) err.printraw("=");_ _195c239_ _< + return_ _sigsetmask(SIGUNBLOCK, set, NULL);_ _---_ _+ return_ _thrsigsetmask(SIGUNBLOCK,_ _set, NULL); 199c243_ _< +_ _jiosnprintf(p, buflen,_ _--- +_ _jiosnprintf(buf, buflen, 210c254_ _< +_ _jiosnprintf(buf,_ _buflen, "dbx - %d",_ _os::currentprocessid());_ _--- +_ _jiosnprintf(buf, 80, "dbx - %d",_ _os::currentprocessid()); < ---_ _old/src/os/windows/vm/oswindows.cpp 2015-11-18_ _19:05:31.977186818 +0100_ _< +++_ _new/src/os/windows/vm/oswindows.cpp_ _2015-11-18 19:05:31.829186815_ _+0100_ _--- diff_ _--git_ _a/src/os/windows/vm/oswindows.cpp_ _b/src/os/windows/vm/oswindows.cpp_ _---_ _a/src/os/windows/vm/oswindows.cpp_ _+++_ _b/src/os/windows/vm/oswindows.cpp_ _@@_ _-4005,7 +4005,7 @@ }_ _-int_ _os::messagebox(const char* title,_ _const char* message) {_ _+bool_ _os::messagebox(const char* title,_ _const char* message) {_ _int result =_ _MessageBox(NULL,_ _message, title,_ _MBYESNO |_ _MBICONERROR | MBSYSTEMMODAL_ _|_ _MBDEFAULTDESKTOPONLY);_ _return result == IDYES;_ _230a286_ _-_ _232c288_ _< +_ _jiosnprintf(p, buflen, ---_ _+_ _jiosnprintf(buf, buflen,_ _On 11/18/15_ _1:15 PM,_ _Sebastian Sickelmann_ _wrote: Hi,_ _Coleen found some places where I_ _missed some_ _refactoring due to the_ _rebase_ _of the initial patch._ _Thanks_ _for reporting it_ _to me._ _I hope_ _this here is fine_ _now on every_ _platform:_ _http://cr.openjdk.java.net/~sebastian/8136978/webrev.02/_ _<_ _http://cr.openjdk.java.net/%7Esebastian/8136978/webrev.02/> <_ _http://cr.openjdk.java.net/%7Esebastian/8136978/webrev.02/> Sorry for the inconvenience. -- Sebastian

More information about the hotspot-runtime-dev mailing list