RFR(S): 8020701: Avoid crashes in WatcherThread
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Jul 18 07:01:48 PDT 2013
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
>>>>
More information about the serviceability-dev
mailing list