RFR(S): 8020701: Avoid crashes in WatcherThread

[Rickard Bäckman]

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
>