RFR: 8136978 Much nearly duplicated code for vmError support

[Thomas Stüfe]

Hi David,

On Fri, Nov 20, 2015 at 6:05 AM, David Holmes <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.

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.



> 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
>>>>>
>>>>
>>>>
>>