RFR(S): 8020701: Avoid crashes in WatcherThread

[Daniel D. Daugherty]

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