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 >