RFR: 8136978 Much nearly duplicated code for vmError support
Thomas Stüfe
thomas.stuefe at gmail.com
Fri Nov 20 09:26:30 UTC 2015
Hi David,
On Fri, Nov 20, 2015 at 8:47 AM, David Holmes <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>> 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
solaris thr_sigsetmask
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.
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