RFR: 8136978 Much nearly duplicated code for vmError support
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Nov 20 18:59:53 UTC 2015
This is nice. It does accomplish removing the duplicated code with the
most direct change. You should use #elif in os_posix.cpp though.
And I think one of the replies said that solaris can use pthread_sigmask
too, although I don't know if this is equivalent myself.
+int os::Posix::unblock_thread_signal_mask(const sigset_t *set) {
+#ifndef TARGET_OS_FAMILY_solaris
+ return pthread_sigmask(SIG_UNBLOCK, set, NULL);
+#else
+ return thr_sigsetmask(SIG_UNBLOCK, set, NULL);
+#endif
+}
Now we'll see what the other's think.
Coleen
On 11/20/15 1:52 PM, Sebastian Sickelmann wrote:
> Hi,
>
> I updated the webrev
>
> http://cr.openjdk.java.net/~sebastian/8136978/webrev.05
> <http://cr.openjdk.java.net/%7Esebastian/8136978/webrev.04>
>
> I now like the patch much more than all created before for this issue.
> Unfortunately it is now nearer to the not chosen alternative solutions from
>
> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-October/016048.html
> and
>
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-October/020249.html
>
>
> This is not a real problem. I learned much good thinks in the last few
> weeks.
>
> --
> Sebastian
>
> On 11/20/2015 05:38 PM, 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
>>> <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::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.
>>>
>>>
>>> I understand that, but I would have liked a version of
>>> ucontext_get_pc/ucontext_set_pc outside an os-specific namespace:
>>>
>>> Now you have to use os::Aix::ucontext_get_pc(),
>>> os::Linux::ucontext_get_pc() 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::uncontext_get_pc or just os::ucontext_get_pc().
>>>
>>> In my mind os::Posix::ucontext_get_pc() makes more sense, because
>>> there is no windows version for this, and ucontext_t is Posix... but
>>> I don't have strong emotions.
>> 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::ucontext_get_pc() be in
>> os_posix.hpp and have ifdefs to go to
>> os::Linux/Solaris/etc/::ucontext_get_pc? Like:
>>
>> static address ucontext_get_pc(ucontext_t* ctx) { #if
>> TARGET_OS_FAMILY_solaris return Solaris::ucontext_get_pc(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::ucontext_get_pc().
>>
>> Thanks,
>> Coleen
>>> Kind Regards, Thomas
>>>
>>>
>>>
>>> Solaris can use pthread_sigmask 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> 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>
>>> <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>>>> 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>
>>> 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
>>> <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::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/
>>> <http://cr.openjdk.java.net/%7Esebastian/8136978/webrev.02/>
>>> Sorry for the
>>> inconvenience.
>>> --
>>> Sebastian
>>>
>>>
More information about the hotspot-runtime-dev
mailing list