RFR: 8136978 Much nearly duplicated code for vmError support
David Holmes
david.holmes at oracle.com
Mon Nov 23 02:11:31 UTC 2015
Hi Sebastian,
On 21/11/2015 6:08 AM, Sebastian Sickelmann wrote:
> Hi,
>
> yes of course #elif is much nicer.
> Sorry David, i have read your hint regarding pthread and solaris but i
> thought about aix and did not recognize the solaris in your mail at this
> moment.
>
> So now there are two enhancements, JDK-8143395 and JDK-8143558. I will
> read into the topic of signals mask for pthread and solaris and aix and
> maybe continue working on those.
>
> The updated webrev is here:
>
> http://cr.openjdk.java.net/~sebastian/8136978/webrev.06
I'm still mulling over the ucontext changes ... need to check something
with Thomas.
src/os/posix/vm/vmError_posix.cpp:
If this is the only code that calls
os::Posix::unblock_thread_signal_mask, and this is itself POSIX code,
then we don't actually need os::Posix::unblock_thread_signal_mask, but
can just put in the pthread_sigmask call directly.
Thanks,
David
> --
> Sebastian
>
> On 11/20/2015 07:59 PM, Coleen Phillimore wrote:
>>
>> 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