RFR: 8136978 Much nearly duplicated code for vmError support
Sebastian Sickelmann
sebastian.sickelmann at gmx.de
Fri Nov 20 13:51:13 UTC 2015
Hi,
On 11/20/2015 01:44 PM, David Holmes 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.
No need to be sorry. I simply missed this suggestion. I will prepare a
new webrev for this.
>
> Solaris can use pthread_sigmask so no need for special casing.
I changed it already in webrev.04 , if i not missed another place. I
just introduced a new issue in bugs.openjdk.java.net because the other
place in hotspot is not strictly related to the remove of duplicated
code for vmError support.
--
Sebastian
> Thanks,
> David
>
>> http://cr.openjdk.java.net/~sebastian/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>> 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>>> 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
>>>> 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
>>>> 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/
>>>> Sorry for the inconvenience.
>>>> --
>>>> Sebastian
>
More information about the hotspot-runtime-dev
mailing list