RFR: 8136978 Much nearly duplicated code for vmError support
Thomas Stüfe
thomas.stuefe at gmail.com
Tue Nov 24 16:09:37 UTC 2015
Hi all,
http://cr.openjdk.java.net/~sebastian/8136978/webrev.06
1) build error, there is a typo in os_aix.cpp (void bool
start_debugging()), please remove the "void"
2) os::Posix::unblock_thread_signal_mask() now uses pthread_sigmask for all
Unices. Are we sure pthread_sigmask 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 "thr_xx"
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>
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 <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.
>>>
>>
> 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 os_posix_<cpu>.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::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().
>>
>
> This seems like a reasonable compromise to me. The API is in the Posix
> namespace, the implementation remains in the os_<os>_<cpu>.cpp file. The
> forwarding function in os_posix.cpp it a tolerable "evil".
>
> Thanks,
> David
>
> 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