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

David Holmes david.holmes at oracle.com
Fri Nov 20 07:47:04 UTC 2015

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>> 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 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:

I hadn't noticed the:

os::Posix::ucontext_get_pc

which I assume is more CPU dependent than OS dependent?

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 os_posix.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

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 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/ Sorry for the inconvenience. -- Sebastian

More information about the hotspot-runtime-dev mailing list