RFR: 8136978 Much nearly duplicated code for vmError support
David Holmes
david.holmes at oracle.com
Fri Nov 20 12:44:50 UTC 2015
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::ucontext_get_pc 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.
Solaris can use pthread_sigmask so no need for special casing.
Thanks,
David
> http://cr.openjdk.java.net/~sebastian/8136978/webrev.04 I moved the
> unblocking of signals to os_posix.cpp and renamed it to
> unblock_thread_signal_mask The use of sigthreadmask is now replaced with pthread_sigmask. For the remaining sigthreadmask in the codebase I created JDK-8143395.
>
> I also removed format_debug_message and refactored the content into start_debugging.
>
> Can some tell me why this "do {} while (true)" in VMError::show_message_box 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>> 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>>> 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/os_posix.cpp file as it is supposed
>>> to be a shared implementation. Otherwise it should just
>>> be in the os:: namespace and have an os_<os>.cpp
>>> implementation - as you have here. I think this
>>> is a borderline case. ucontext_t is Posix, so it fits into
>>> os_posix.hpp nicely, but the implementation for
>>> ucontext_get/set_pc() is deeply platform dependend and must
>>> live in platform specific files. Sorry I was referring to: +
>>> int os::Posix::set_thread_signal_mask_unblocked(const sigset_t *set)
>>> { + return pthread_sigmask(SIG_UNBLOCK, set, NULL); + } Oh,
>>> that. We use different APIs for setting thread signal masks: aix
>>> sigthreadmask linux,bsd pthread_sigmask solaristhr_sigsetmask
>> Solaris can use pthread_sigmask too. Given what I've been doing with
>> pthread_get/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 pthread_sigmask(), 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 pthread_sigmask() all over the place in os_aix.cpp,
>>> so this needs checking. Maybe we can simply switch to
>>> pthread_sigmask() for AIX.
>> This suggests pthread_sigmask == sigthreadmask:
>> http://www-01.ibm.com/support/knowledgecenter/ssw_aix_53/com.ibm.aix.basetechref/doc/basetrf1/pthread_sigmask.htm%23i08219704tanab
>> Cheers, David -----
>>> For the implementation this means we either live with 2-3 small
>>> #ifdef sections in os_posix.cpp or we introduce os_posix_<os>.cpp. I
>>> strongly prefer the first variant, but that is a matter of taste.
>>> I hadn't noticed the: os::Posix::ucontext_get_pc which I
>>> assume is more CPU dependent than OS dependent? Both. ucontext_t on
>>> AIX looks different than on Linux PowerPC. Thats why the
>>> implementations live in <os>_<cpu>. We *could* stretch this a
>>> bit by including CONTEXT (the windows variant of
>>> uncontext_t), typedef a common type for CONTEXT/ucontext_t
>>> and add implementations for windows too... but I think this
>>> is unnecessary. Or, we could put all implementations into
>>> os_posix.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 Regards, Thomas ------
>>> Also os::format_debug_message looks like a candidate for
>>> a shared implementation as well. Agreed
>>> here. I even would prefer os::format_debug_message() to be
>>> folded somehow into os::start_debugging() 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::message_box 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
>>> thr_sigsetmask on
>>> solaris and a couple places
>>> os::format_debug_message() you had 'p'
>>> rather than 'buf' and one place had 'buflen' rather than
>>> 80. Those places
>>> os::start_debugging(), 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/os_solaris.cpp
>>> 2015-11-18 19:05:31.209186804 +0100
>>> < +++ new/src/os/solaris/vm/os_solaris.cpp
>>> 2015-11-18 19:05:31.129186803 +0100
>>> --- diff --git
>>> a/src/os/solaris/vm/os_solaris.cpp
>>> b/src/os/solaris/vm/os_solaris.cpp ---
>>> a/src/os/solaris/vm/os_solaris.cpp +++
>>> b/src/os/solaris/vm/os_solaris.cpp @@
>>> -3611,7 +3611,7 @@ void
>>> os::print_statistics() { }
>>> -int os::message_box(const char* title,
>>> const char* message) {
>>> +bool os::message_box(const char* title,
>>> const char* message) {
>>> int i;
>>> fdStream
>>> err(defaultStream::error_fd()); for
>>> (i = 0; i < 78; i++) err.print_raw("=");
>>> 195c239 < + return
>>> sigsetmask(SIG_UNBLOCK, set, NULL); ---
>>> + return thr_sigsetmask(SIG_UNBLOCK,
>>> set, NULL); 199c243
>>> < + jio_snprintf(p, buflen,
>>> --- +
>>> jio_snprintf(buf, buflen, 210c254
>>> < + jio_snprintf(buf, buflen, "dbx - %d",
>>> os::current_process_id());
>>> --- +
>>> jio_snprintf(buf, 80, "dbx - %d",
>>> os::current_process_id()); < ---
>>> old/src/os/windows/vm/os_windows.cpp 2015-11-18
>>> 19:05:31.977186818 +0100
>>> < +++ new/src/os/windows/vm/os_windows.cpp
>>> 2015-11-18 19:05:31.829186815 +0100
>>> --- diff --git
>>> a/src/os/windows/vm/os_windows.cpp
>>> b/src/os/windows/vm/os_windows.cpp ---
>>> a/src/os/windows/vm/os_windows.cpp +++
>>> b/src/os/windows/vm/os_windows.cpp @@
>>> -4005,7 +4005,7 @@ }
>>> -int os::message_box(const char* title,
>>> const char* message) {
>>> +bool os::message_box(const char* title,
>>> const char* message) {
>>> int result = MessageBox(NULL,
>>> message, title,
>>> MB_YESNO |
>>> MB_ICONERROR | MB_SYSTEMMODAL
>>> | MB_DEFAULT_DESKTOP_ONLY);
>>> return result == IDYES;
>>> 230a286 -
>>> 232c288 < +
>>> jio_snprintf(p, buflen, ---
>>> + jio_snprintf(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