RFR(S): 8020701: Avoid crashes in WatcherThread

[Daniel D. Daugherty]

On 7/18/13 7:58 AM, Rickard Bäckman wrote:
> Fixed them all, will start jprt-jobs.

Thanks!


> Didn't know we were so strict on the 80 columns, considering all the other code in those files… :)

Most folks aren't, but I prefer that we don't add to the problem
when we add new code.

I personally take the opportunity to fix such things if I'm
modifying such a line in the course of a bug fix, but that is a
matter of personal taste. Slowly, we'll reduce the "style
violations" over a very long time...

Dan


>
> Thanks
> /R
>
> On Jul 18, 2013, at 3:09 PM, Daniel D. Daugherty wrote:
>
>>> http://cr.openjdk.java.net/~rbackman/8020701.1/
>> Thumbs up!
>>
>> src/os/posix/vm/os_posix.cpp
>>     Can you reformat the new lines that are over 80 cols?
>>     No need to re-review that change if you do it.
>>
>> src/os/posix/vm/os_posix.hpp
>>     No comments.
>>
>> src/os/windows/vm/os_windows.cpp
>>     The os_posix.cpp funcation has these:
>>
>>       274   assert(Thread::current()->is_Watcher_thread(), "Only for WatcherThread");
>>       275 assert(!WatcherThread::watcher_thread()->has_crash_protection(), "crash_protection already set?");
>>
>>     but the Windows version of WatcherThreadCrashProtection::call()
>>     does not. Any particular reason for the difference?
>>
>>     Sorry I missed this in the first round.
>>
>> src/os/windows/vm/os_windows.hpp
>>     No comments.
>>
>> src/os_cpu/bsd_x86/vm/os_bsd_x86.cpp
>> src/os_cpu/linux_sparc/vm/os_linux_sparc.cpp
>> src/os_cpu/linux_x86/vm/os_linux_x86.cpp
>> src/os_cpu/solaris_sparc/vm/os_solaris_sparc.cpp
>> src/os_cpu/solaris_x86/vm/os_solaris_x86.cpp
>>     No comments.
>>
>> src/share/vm/runtime/mutex.cpp
>>     The new assert line is well past 80 cols.
>>     Again, no need to re-review.
>>
>> src/share/vm/runtime/os.cpp
>>     line 600: // since os::malloc can be called when the libjvm.{dll.so} is
>>         typo: '{dll.so}'
>>            -> '{dll,so}'
>>
>>     line 601: // loaded and we don't have a thread yet.
>>         Please change "loaded" -> "first loaded"
>>
>> src/share/vm/runtime/os.hpp
>>     No comments.
>>
>> src/share/vm/runtime/thread.cpp
>>     No comments.
>>
>> src/share/vm/runtime/thread.hpp
>>     No comments.
>>
>> Dan
>>
>>
>> On 7/18/13 4:37 AM, Rickard Bäckman wrote:
>>> Hi,
>>>
>>> here is the updated webrev containing Dans suggested changes.
>>> I also had to change the assert check in os::malloc, we discovered it wasn't safe to check Thread::current() since os::malloc() can be called when the libjvm is loaded / initialized and we don't have a thread yet.
>>> Changing to logic a bit to require that the WatcherThread exists before looking up the thread and doing it by ThreadLocalStorage::get_thread_slow() makes it safe.
>>>
>>> That is the only code change, other changes are comments or newlines in code (thread.hpp).
>>>
>>> See the updated webrev:
>>> http://cr.openjdk.java.net/~rbackman/8020701.1/
>>>
>>> Thanks
>>> /R
>>>
>>>
>>> On Jul 17, 2013, at 8:12 PM, Rickard Bäckman wrote:
>>>
>>>> On Jul 17, 2013, at 7:03 PM, Daniel D. Daugherty wrote:
>>>>
>>>>> On 7/17/13 5:58 AM, Rickard Bäckman wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> can I please have reviews for the following change?
>>>>>>
>>>>>> We are adding a mechanism for avoiding some crashes in the WatcherThread using different OS-specific methods.
>>>>>>
>>>>>> Webrev: http://cr.openjdk.java.net/~rbackman/8020701/webrev/
>>>>>>
>>>>>> Thanks
>>>>>> /R
>>>>> Thumbs up! The bug mentions a few tests where you think this new
>>>>> infrastructure will help diagnose the failures modes for those tests.
>>>>> However, are there any new tests targeted to exercise this code?
>>>> Not diagnose the failures, but avoid the crashes they are currently causing.
>>>> Since at least one of them haven't been fixed yet (working on it) it will work as
>>>> a good verifier at the moment. When the bug is gone, it is harder. It is a bit hard
>>>> to write a test that causes the VM to crash at random but specific places. Eventually
>>>> we could do something with the whitebox api, but the crashes would only happen at
>>>> very specific places in that case making them a bit less interesting.
>>>>
>>>>> src/share/vm/runtime/os.hpp
>>>>>    No comments.
>>>>>
>>>>> src/share/vm/runtime/os.cpp
>>>>>    No comments.
>>>>>
>>>>> src/share/vm/runtime/thread.hpp
>>>>>    line 756: void set_crash_protection(os::WatcherThreadCrashProtection* crash_protection) { assert(Thread::current()->is_Watcher_thread(), "Can only be set by WatcherThread"); _crash_protection = crash_protection; }
>>>>>        This line is ridiculously long. Please reformat it to fit in 80 cols.
>>>> Ok
>>>>
>>>>> src/share/vm/runtime/thread.cpp
>>>>>    No comments.
>>>>>
>>>>> src/os/posix/vm/os_posix.hpp
>>>>>    Nice comment. Hopefully it will keep anyone from doing something
>>>>>    dangerous in the future.
>>>> :)
>>>>
>>>>> src/os/posix/vm/os_posix.cpp'
>>>>>    Please add the following line above 268:
>>>>>
>>>>>    * See the caveats for this class in os_posix.hpp.
>>>> Ok
>>>>
>>>>> src/os/windows/vm/os_windows.hpp
>>>>>    No comments.
>>>>>
>>>>> src/os/windows/vm/os_windows.cpp
>>>>>    line 4692  * Protects the callback call so that S jumps back into this method
>>>>>        Stale comment? What is 'S'?
>>>> typo. Should be something like raised EXCEPTIONS causes a jump back into this method.
>>>>
>>>>>    Please add the following line above 4692:
>>>>>
>>>>>    * See the caveats for this class in os_windows.hpp.
>>>>>
>>>>> src/os_cpu/bsd_x86/vm/os_bsd_x86.cpp
>>>>>    line 405:   // (no destructors run)
>>>>>        Please change to "(no destructors can be run)"
>>>> Ok
>>>>
>>>>> src/os_cpu/linux_sparc/vm/os_linux_sparc.cpp
>>>>>    line 557:   // (no destructors run)
>>>>>        Please change to "(no destructors can be run)"
>>>>>
>>>>> src/os_cpu/linux_x86/vm/os_linux_x86.cpp
>>>>>    line 229:   // (no destructors run)
>>>>>        Please change to "(no destructors can be run)"
>>>>>
>>>>> src/os_cpu/solaris_sparc/vm/os_solaris_sparc.cpp
>>>>>    line 319:   // (no destructors run)
>>>>>        Please change to "(no destructors can be run)"
>>>>>
>>>>> src/os_cpu/solaris_x86/vm/os_solaris_x86.cpp
>>>>>    line 378:   // (no destructors run)
>>>>>        Please change to "(no destructors can be run)"
>>>>>
>>>>> src/share/vm/runtime/mutex.cpp
>>>>>    No comments.
>>>>>
>>>>> Dan
>>>>>
>>>> Thanks for the review!
>>>>
>>>> /R
>>>>