RFR: 8136978 Much nearly duplicated code for vmError support

[David Holmes]

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.

Also os::format_debug_message looks like a candidate for a shared 
implementation as well.

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