RFR: 8136978 Much nearly duplicated code for vmError support
David Holmes
david.holmes at oracle.com
Thu Nov 26 02:15:53 UTC 2015
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
-----
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