RFR: 8136978 Much nearly duplicated code for vmError support
Sebastian Sickelmann
sebastian.sickelmann at gmx.de
Wed Nov 25 15:50:53 UTC 2015
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 <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
> <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 <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
>> <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