RFR(S): 8020701: Avoid crashes in WatcherThread

[Rickard Bäckman]

Fixed them all, will start jprt-jobs.

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

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