RFR: 8136978 Much nearly duplicated code for vmError support
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Nov 20 16:38:47 UTC 2015
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>> 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.
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().
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> 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>
> 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