RFR: 8136978 Much nearly duplicated code for vmError support
Thomas Stüfe
thomas.stuefe at gmail.com
Thu Nov 26 07:44:39 UTC 2015
Hi David, Sebastian,
On Thu, Nov 26, 2015 at 3:15 AM, David Holmes <david.holmes at oracle.com>
wrote:
> One of my comments was overlooked in the process here:
>
> 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.
>
> ---
>
> That would have made it even cleaner IMHO.
>
> David
> -----
>
>
I disagree. But I would make the function more useful, the way it is now it
does not add anything to plain pthread_sigmask() :
- I would make this function able to both block and unblock, no reason to
only support one side.
- I would add error handling for EINVAL
- I would feel better about continue using sigthreadmask for AIX until we
know for sure that pthread_sigmask is equivalent to sigthreadmask() for all
supported AIX versions. I have conflicting informations here.
For comparison, here is our (SAP) variant in os_posix.cpp, which we added
some time ago:
bool os::Posix::set_thread_signal_mask(int how, const sigset_t* set,
sigset_t* oset) {
const int rc =
#ifdef _AIX
::sigthreadmask(how, set, oset);
#else
::pthread_sigmask(how, set, oset);
#endif
// Note: in case of error, pthread_sigmask() returns error number,
// sigthreadmask returns -1 and sets global errno. But both return
// 0 in case of success.
return rc == 0 ? true : false;
}
Kind Regards, Thomas
>
>
> On 26/11/2015 1:58 AM, Coleen Phillimore wrote:
>
>>
>> Sebastian, Thank you for the cleanup. I'll start the push.
>>
>> Coleen
>>
>> On 11/25/15 10:50 AM, Sebastian Sickelmann wrote:
>>
>>> Hi Coleen,
>>>
>>> I have create the changeset and uploaded it to
>>> http://cr.openjdk.java.net/~sebastian/8136978/webrev.08
>>>
>>> I inserted a reviewed-by line with Thomas, David and you.
>>> If someone do not want to be included as reviewer please mail to
>>> Coleen to stop the push process. I will than update the changeset
>>> accordingly.
>>>
>>> Thanks to all for all your review. It was really helpful to create a
>>> good cleanup result.
>>>
>>> --
>>> Sebastian
>>>
>>> On 11/25/2015 08:02 AM, Thomas Stüfe wrote:
>>>
>>>> Hi Sebastian,
>>>>
>>>> assuming the build error fix for AIX is the only change for webrev
>>>> 07, I'm fine with it.
>>>>
>>>> The question about thr_setsigmask vs pthread_sigmask on Solaris
>>>> remains and should be answered by a Solaris expert IMHO.
>>>>
>>>> Otherwise, thank you for this cleanup!
>>>>
>>>> ..Thomas
>>>>
>>>> On Tue, Nov 24, 2015 at 5:54 PM, Sebastian Sickelmann
>>>> <sebastian.sickelmann at gmx.de> wrote:
>>>>
>>>> Hi,
>>>>
>>>> sorry for the typo.
>>>>
>>>> The new webrev is here:
>>>>
>>>> http://cr.openjdk.java.net/~sebastian/8136978/webrev.07
>>>> <http://cr.openjdk.java.net/%7Esebastian/8136978/webrev.07>
>>>>
>>>> I will also execute the mentioned tests on linux.
>>>>
>>>> --
>>>> Sebastian
>>>>
>>>>
>>>> On 11/24/2015 05:09 PM, Thomas Stüfe wrote:
>>>>
>>>>> Hi all,
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~sebastian/8136978/webrev.06
>>>>> <http://cr.openjdk.java.net/%7Esebastian/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>
>>>>> <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>
>>>>>
>>>>> <
>>>>> 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
>>>>> >
>>>>> 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/%7Esebastian/8136978/webrev.02>
>>>>> 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