RFR: 8136978 Much nearly duplicated code for vmError support
Sebastian Sickelmann
sebastian.sickelmann at gmx.de
Tue Nov 24 16:54:53 UTC 2015
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
> <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