RFR: 8136978 Much nearly duplicated code for vmError support
David Holmes
david.holmes at oracle.com
Fri Nov 20 09:49:11 UTC 2015
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