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>
--- a/hotspot/src/os/aix/vm/os_aix.cpp Wed May 11 00:40:59 2016 +0000
+++ b/hotspot/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/hotspot/src/os/bsd/vm/os_bsd.cpp Wed May 11 00:40:59 2016 +0000
+++ b/hotspot/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/hotspot/src/os/linux/vm/os_linux.cpp Wed May 11 00:40:59 2016 +0000
+++ b/hotspot/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(¤t);
- pthread_sigmask(SIG_SETMASK, NULL, ¤t);
- assert(!sigismember(¤t, SR_signum), "SR signal should not be blocked!");
+ sigset_t current;
+ sigemptyset(¤t);
+ pthread_sigmask(SIG_SETMASK, NULL, ¤t);
+ assert(!sigismember(¤t, 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/hotspot/src/os/solaris/vm/os_solaris.cpp Wed May 11 00:40:59 2016 +0000
+++ b/hotspot/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/hotspot/src/os/windows/vm/os_windows.cpp Wed May 11 00:40:59 2016 +0000
+++ b/hotspot/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/hotspot/src/os/windows/vm/os_windows.hpp Wed May 11 00:40:59 2016 +0000
+++ b/hotspot/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/hotspot/src/share/vm/runtime/thread.hpp Wed May 11 00:40:59 2016 +0000
+++ b/hotspot/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/hotspot/src/share/vm/runtime/vmThread.cpp Wed May 11 00:40:59 2016 +0000
+++ b/hotspot/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/hotspot/src/share/vm/runtime/vmThread.hpp Wed May 11 00:40:59 2016 +0000
+++ b/hotspot/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; }