Fixed them all, will start jprt-jobs. Didn't know we were so strict on the 80 columns, considering all the other code in those files… :)
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 >>> >
