RFR: 8136978 Much nearly duplicated code for vmError support
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Nov 25 15:58:21 UTC 2015
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
>>> 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/>
>>> Sorry for the inconvenience.
>>> -- Sebastian
>>>
>>>
>>>
>>>
>>
>>
>
More information about the hotspot-runtime-dev
mailing list