RFR: 8136978 Much nearly duplicated code for vmError support
Thomas Stüfe
thomas.stuefe at gmail.com
Wed Nov 25 10:48:42 UTC 2015
Hi David,
On Wed, Nov 25, 2015 at 10:45 AM, David Holmes <david.holmes at oracle.com>
wrote:
> On 25/11/2015 5:02 PM, 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.
>>
>
> I have answered this a number of times already (though not claiming the
> Solaris expert label :) ). See for example:
>
> https://docs.oracle.com/cd/E19455-01/806-0630/6j9vkb8hh/index.html
>
> "As POSIX threads and Solaris threads are fully compatible even within the
> same process ..."
>
> Use of pthread_sigmask is fine.
>
> We tried to convert from thr_ to pthread_ APIs a few years back but the
> job got unmanageable (due to issues with T1 and T2 thread libraries) and
> had to be dropped. thr_create allows a thread to be started in the
> suspended state, but otherwise a simple replacement would work. Such a
> change is worth looking at again now we have cleaned out a lot of the
> ancient crud eg T1 stuff.
>
> Thanks,
> David
>
>
Thank you for clarifying!
...Thomas
> Otherwise, thank you for this cleanup!
>>
>> ..Thomas
>>
>> On Tue, Nov 24, 2015 at 5:54 PM, Sebastian Sickelmann
>> <sebastian.sickelmann at gmx.de <mailto: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
>>
>> 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 <mailto: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
>>> <<mailto:david.holmes at oracle.com>david.holmes at oracle.com
>>> <mailto: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>>
>>> <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 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>>>
>>> <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
>>> <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>
>>>
>>> <
>>> 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>
>>> <
>>> 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/>
>>>
>>> <
>>> http://cr.openjdk.java.net/%7Esebastian/8136978/webrev.02/>
>>> Sorry
>>> for the inconvenience.
>>> --
>>> Sebastian
>>>
>>>
>>>
>>>
>>>
>>
>>
More information about the hotspot-runtime-dev
mailing list