changeset 11235:aa11081b8307

8154715: Missing destructor and/or TLS clearing calls for terminating threads Summary: clear TLS on return from thread->run() unless already done. Prohibit asynchronous thread deletion. Reviewed-by: stefank, sspitsyn Contributed-by: Brian Gardner <openjdk@getsnappy.com>
author dholmes
date Wed, 11 May 2016 01:02:28 -0400
parents b2f17e1250fd
children 5560c6f1449d
files src/os/aix/vm/os_aix.cpp src/os/bsd/vm/os_bsd.cpp src/os/linux/vm/os_linux.cpp src/os/solaris/vm/os_solaris.cpp src/os/windows/vm/os_windows.cpp src/os/windows/vm/os_windows.hpp src/share/vm/runtime/thread.hpp src/share/vm/runtime/vmThread.cpp src/share/vm/runtime/vmThread.hpp
diffstat 9 files changed, 116 insertions(+), 52 deletions(-) [+]
line wrap: on
line diff
--- a/src/os/aix/vm/os_aix.cpp	Wed May 11 00:40:59 2016 +0000
+++ b/src/os/aix/vm/os_aix.cpp	Wed May 11 01:02:28 2016 -0400
@@ -777,7 +777,7 @@
 // create new thread
 
 // Thread start routine for all newly created threads
-static void *java_start(Thread *thread) {
+static void *thread_native_entry(Thread *thread) {
 
   // find out my own stack dimensions
   {
@@ -838,6 +838,15 @@
   log_info(os, thread)("Thread finished (tid: " UINTX_FORMAT ", kernel thread id: " UINTX_FORMAT ").",
     os::current_thread_id(), (uintx) kernel_thread_id);
 
+  // If a thread has not deleted itself ("delete this") as part of its
+  // termination sequence, we have to ensure thread-local-storage is
+  // cleared before we actually terminate. No threads should ever be
+  // deleted asynchronously with respect to their termination.
+  if (Thread::current_or_null_safe() != NULL) {
+    assert(Thread::current_or_null_safe() == thread, "current thread is wrong");
+    thread->clear_thread_current();
+  }
+
   return 0;
 }
 
@@ -902,7 +911,7 @@
   pthread_attr_setstacksize(&attr, stack_size);
 
   pthread_t tid;
-  int ret = pthread_create(&tid, &attr, (void* (*)(void*)) java_start, thread);
+  int ret = pthread_create(&tid, &attr, (void* (*)(void*)) thread_native_entry, thread);
 
 
   char buf[64];
@@ -993,11 +1002,14 @@
 void os::free_thread(OSThread* osthread) {
   assert(osthread != NULL, "osthread not set");
 
-  if (Thread::current()->osthread() == osthread) {
-    // Restore caller's signal mask
-    sigset_t sigmask = osthread->caller_sigmask();
-    pthread_sigmask(SIG_SETMASK, &sigmask, NULL);
-   }
+  // We are told to free resources of the argument thread,
+  // but we can only really operate on the current thread.
+  assert(Thread::current()->osthread() == osthread,
+         "os::free_thread but not current thread");
+
+  // Restore caller's signal mask
+  sigset_t sigmask = osthread->caller_sigmask();
+  pthread_sigmask(SIG_SETMASK, &sigmask, NULL);
 
   delete osthread;
 }
--- a/src/os/bsd/vm/os_bsd.cpp	Wed May 11 00:40:59 2016 +0000
+++ b/src/os/bsd/vm/os_bsd.cpp	Wed May 11 01:02:28 2016 -0400
@@ -665,7 +665,7 @@
 #endif
 
 // Thread start routine for all newly created threads
-static void *java_start(Thread *thread) {
+static void *thread_native_entry(Thread *thread) {
   // Try to randomize the cache line index of hot stack frames.
   // This helps when threads of the same stack traces evict each other's
   // cache lines. The threads can be either from the same JVM instance, or
@@ -723,6 +723,15 @@
   log_info(os, thread)("Thread finished (tid: " UINTX_FORMAT ", pthread id: " UINTX_FORMAT ").",
     os::current_thread_id(), (uintx) pthread_self());
 
+  // If a thread has not deleted itself ("delete this") as part of its
+  // termination sequence, we have to ensure thread-local-storage is
+  // cleared before we actually terminate. No threads should ever be
+  // deleted asynchronously with respect to their termination.
+  if (Thread::current_or_null_safe() != NULL) {
+    assert(Thread::current_or_null_safe() == thread, "current thread is wrong");
+    thread->clear_thread_current();
+  }
+
   return 0;
 }
 
@@ -781,7 +790,7 @@
 
   {
     pthread_t tid;
-    int ret = pthread_create(&tid, &attr, (void* (*)(void*)) java_start, thread);
+    int ret = pthread_create(&tid, &attr, (void* (*)(void*)) thread_native_entry, thread);
 
     char buf[64];
     if (ret == 0) {
@@ -889,11 +898,14 @@
 void os::free_thread(OSThread* osthread) {
   assert(osthread != NULL, "osthread not set");
 
-  if (Thread::current()->osthread() == osthread) {
-    // Restore caller's signal mask
-    sigset_t sigmask = osthread->caller_sigmask();
-    pthread_sigmask(SIG_SETMASK, &sigmask, NULL);
-  }
+  // We are told to free resources of the argument thread,
+  // but we can only really operate on the current thread.
+  assert(Thread::current()->osthread() == osthread,
+         "os::free_thread but not current thread");
+
+  // Restore caller's signal mask
+  sigset_t sigmask = osthread->caller_sigmask();
+  pthread_sigmask(SIG_SETMASK, &sigmask, NULL);
 
   delete osthread;
 }
--- a/src/os/linux/vm/os_linux.cpp	Wed May 11 00:40:59 2016 +0000
+++ b/src/os/linux/vm/os_linux.cpp	Wed May 11 01:02:28 2016 -0400
@@ -638,7 +638,7 @@
 // create new thread
 
 // Thread start routine for all newly created threads
-static void *java_start(Thread *thread) {
+static void *thread_native_entry(Thread *thread) {
   // Try to randomize the cache line index of hot stack frames.
   // This helps when threads of the same stack traces evict each other's
   // cache lines. The threads can be either from the same JVM instance, or
@@ -690,6 +690,15 @@
   log_info(os, thread)("Thread finished (tid: " UINTX_FORMAT ", pthread id: " UINTX_FORMAT ").",
     os::current_thread_id(), (uintx) pthread_self());
 
+  // If a thread has not deleted itself ("delete this") as part of its
+  // termination sequence, we have to ensure thread-local-storage is
+  // cleared before we actually terminate. No threads should ever be
+  // deleted asynchronously with respect to their termination.
+  if (Thread::current_or_null_safe() != NULL) {
+    assert(Thread::current_or_null_safe() == thread, "current thread is wrong");
+    thread->clear_thread_current();
+  }
+
   return 0;
 }
 
@@ -753,7 +762,7 @@
 
   {
     pthread_t tid;
-    int ret = pthread_create(&tid, &attr, (void* (*)(void*)) java_start, thread);
+    int ret = pthread_create(&tid, &attr, (void* (*)(void*)) thread_native_entry, thread);
 
     char buf[64];
     if (ret == 0) {
@@ -881,18 +890,21 @@
 void os::free_thread(OSThread* osthread) {
   assert(osthread != NULL, "osthread not set");
 
-  if (Thread::current()->osthread() == osthread) {
+  // We are told to free resources of the argument thread,
+  // but we can only really operate on the current thread.
+  assert(Thread::current()->osthread() == osthread,
+         "os::free_thread but not current thread");
+
 #ifdef ASSERT
-    sigset_t current;
-    sigemptyset(&current);
-    pthread_sigmask(SIG_SETMASK, NULL, &current);
-    assert(!sigismember(&current, SR_signum), "SR signal should not be blocked!");
+  sigset_t current;
+  sigemptyset(&current);
+  pthread_sigmask(SIG_SETMASK, NULL, &current);
+  assert(!sigismember(&current, SR_signum), "SR signal should not be blocked!");
 #endif
 
-    // Restore caller's signal mask
-    sigset_t sigmask = osthread->caller_sigmask();
-    pthread_sigmask(SIG_SETMASK, &sigmask, NULL);
-  }
+  // Restore caller's signal mask
+  sigset_t sigmask = osthread->caller_sigmask();
+  pthread_sigmask(SIG_SETMASK, &sigmask, NULL);
 
   delete osthread;
 }
--- a/src/os/solaris/vm/os_solaris.cpp	Wed May 11 00:40:59 2016 +0000
+++ b/src/os/solaris/vm/os_solaris.cpp	Wed May 11 01:02:28 2016 -0400
@@ -725,8 +725,8 @@
 
 static thread_t main_thread;
 
-// Thread start routine for all new Java threads
-extern "C" void* java_start(void* thread_addr) {
+// Thread start routine for all newly created threads
+extern "C" void* thread_native_entry(void* thread_addr) {
   // Try to randomize the cache line index of hot stack frames.
   // This helps when threads of the same stack traces evict each other's
   // cache lines. The threads can be either from the same JVM instance, or
@@ -796,6 +796,15 @@
 
   log_info(os, thread)("Thread finished (tid: " UINTX_FORMAT ").", os::current_thread_id());
 
+  // If a thread has not deleted itself ("delete this") as part of its
+  // termination sequence, we have to ensure thread-local-storage is
+  // cleared before we actually terminate. No threads should ever be
+  // deleted asynchronously with respect to their termination.
+  if (Thread::current_or_null_safe() != NULL) {
+    assert(Thread::current_or_null_safe() == thread, "current thread is wrong");
+    thread->clear_thread_current();
+  }
+
   if (UseDetachedThreads) {
     thr_exit(NULL);
     ShouldNotReachHere();
@@ -1009,7 +1018,7 @@
   osthread->set_lwp_id(-1);
   osthread->set_thread_id(-1);
 
-  status = thr_create(NULL, stack_size, java_start, thread, flags, &tid);
+  status = thr_create(NULL, stack_size, thread_native_entry, thread, flags, &tid);
 
   char buf[64];
   if (status == 0) {
@@ -1221,18 +1230,15 @@
 void os::free_thread(OSThread* osthread) {
   assert(osthread != NULL, "os::free_thread but osthread not set");
 
-
   // We are told to free resources of the argument thread,
   // but we can only really operate on the current thread.
-  // The main thread must take the VMThread down synchronously
-  // before the main thread exits and frees up CodeHeap
-  guarantee((Thread::current()->osthread() == osthread
-             || (osthread == VMThread::vm_thread()->osthread())), "os::free_thread but not current thread");
-  if (Thread::current()->osthread() == osthread) {
-    // Restore caller's signal mask
-    sigset_t sigmask = osthread->caller_sigmask();
-    pthread_sigmask(SIG_SETMASK, &sigmask, NULL);
-  }
+  assert(Thread::current()->osthread() == osthread,
+         "os::free_thread but not current thread");
+
+  // Restore caller's signal mask
+  sigset_t sigmask = osthread->caller_sigmask();
+  pthread_sigmask(SIG_SETMASK, &sigmask, NULL);
+
   delete osthread;
 }
 
--- a/src/os/windows/vm/os_windows.cpp	Wed May 11 00:40:59 2016 +0000
+++ b/src/os/windows/vm/os_windows.cpp	Wed May 11 01:02:28 2016 -0400
@@ -409,8 +409,8 @@
 
 LONG WINAPI topLevelExceptionFilter(struct _EXCEPTION_POINTERS* exceptionInfo);
 
-// Thread start routine for all new Java threads
-static unsigned __stdcall java_start(Thread* thread) {
+// Thread start routine for all newly created threads
+static unsigned __stdcall thread_native_entry(Thread* thread) {
   // Try to randomize the cache line index of hot stack frames.
   // This helps when threads of the same stack traces evict each other's
   // cache lines. The threads can be either from the same JVM instance, or
@@ -459,6 +459,15 @@
     Atomic::dec_ptr((intptr_t*)&os::win32::_os_thread_count);
   }
 
+  // If a thread has not deleted itself ("delete this") as part of its
+  // termination sequence, we have to ensure thread-local-storage is
+  // cleared before we actually terminate. No threads should ever be
+  // deleted asynchronously with respect to their termination.
+  if (Thread::current_or_null_safe() != NULL) {
+    assert(Thread::current_or_null_safe() == thread, "current thread is wrong");
+    thread->clear_thread_current();
+  }
+
   // Thread must not return from exit_process_or_thread(), but if it does,
   // let it proceed to exit normally
   return (unsigned)os::win32::exit_process_or_thread(os::win32::EPT_THREAD, res);
@@ -631,7 +640,7 @@
   HANDLE thread_handle =
     (HANDLE)_beginthreadex(NULL,
                            (unsigned)stack_size,
-                           (unsigned (__stdcall *)(void*)) java_start,
+                           (unsigned (__stdcall *)(void*)) thread_native_entry,
                            thread,
                            initflag,
                            &thread_id);
@@ -670,6 +679,12 @@
 // Free Win32 resources related to the OSThread
 void os::free_thread(OSThread* osthread) {
   assert(osthread != NULL, "osthread not set");
+
+  // We are told to free resources of the argument thread,
+  // but we can only really operate on the current thread.
+  assert(Thread::current()->osthread() == osthread,
+         "os::free_thread but not current thread");
+
   CloseHandle(osthread->thread_handle());
   CloseHandle(osthread->interrupt_event());
   delete osthread;
--- a/src/os/windows/vm/os_windows.hpp	Wed May 11 00:40:59 2016 +0000
+++ b/src/os/windows/vm/os_windows.hpp	Wed May 11 01:02:28 2016 -0400
@@ -36,7 +36,7 @@
 
 class win32 {
   friend class os;
-  friend unsigned __stdcall java_start(class Thread*);
+  friend unsigned __stdcall thread_native_entry(class Thread*);
 
  protected:
   static int    _vm_page_size;
--- a/src/share/vm/runtime/thread.hpp	Wed May 11 00:40:59 2016 +0000
+++ b/src/share/vm/runtime/thread.hpp	Wed May 11 01:02:28 2016 -0400
@@ -96,6 +96,7 @@
 //       - GangWorker
 //       - GCTaskThread
 //   - JavaThread
+//     - various subclasses eg CompilerThread, ServiceThread
 //   - WatcherThread
 
 class Thread: public ThreadShadow {
@@ -314,8 +315,7 @@
 
   // Manage Thread::current()
   void initialize_thread_current();
-  private:
-  void clear_thread_current(); // needed for detaching JNI threads
+  void clear_thread_current(); // TLS cleanup needed before threads terminate
 
   public:
   // thread entry point
@@ -743,6 +743,11 @@
   // Constructor
   WatcherThread();
 
+  // No destruction allowed
+  ~WatcherThread() {
+    guarantee(false, "WatcherThread deletion must fix the race with VM termination");
+  }
+
   // Tester
   bool is_Watcher_thread() const                 { return true; }
 
--- a/src/share/vm/runtime/vmThread.cpp	Wed May 11 00:40:59 2016 +0000
+++ b/src/share/vm/runtime/vmThread.cpp	Wed May 11 01:02:28 2016 -0400
@@ -226,16 +226,12 @@
   }
 }
 
-
 VMThread::VMThread() : NamedThread() {
   set_name("VM Thread");
 }
 
 void VMThread::destroy() {
-  if (_vm_thread != NULL) {
-    delete _vm_thread;
-    _vm_thread = NULL;      // VM thread is gone
-  }
+  _vm_thread = NULL;      // VM thread is gone
 }
 
 void VMThread::run() {
@@ -308,9 +304,9 @@
     _terminate_lock->notify();
   }
 
-  // Deletion must be done synchronously by the JNI DestroyJavaVM thread
-  // so that the VMThread deletion completes before the main thread frees
-  // up the CodeHeap.
+  // We are now racing with the VM termination being carried out in
+  // another thread, so we don't "delete this". Numerous threads don't
+  // get deleted when the VM terminates
 
 }
 
--- a/src/share/vm/runtime/vmThread.hpp	Wed May 11 00:40:59 2016 +0000
+++ b/src/share/vm/runtime/vmThread.hpp	Wed May 11 01:02:28 2016 -0400
@@ -104,6 +104,12 @@
   // Constructor
   VMThread();
 
+  // No destruction allowed
+  ~VMThread() {
+    guarantee(false, "VMThread deletion must fix the race with VM termination");
+  }
+
+
   // Tester
   bool is_VM_thread() const                      { return true; }
   bool is_GC_thread() const                      { return true; }