Re: RFR(S): 8020701: Avoid crashes in WatcherThread

Daniel D. Daugherty Thu, 18 Jul 2013 06:12:04 -0700

> 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 forWatcherThread");275assert(!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

Re: RFR(S): 8020701: Avoid crashes in WatcherThread

Reply via email to