--- old/src/hotspot/os/linux/os_linux.cpp 2018-12-20 23:34:57.626098938 -0500 +++ new/src/hotspot/os/linux/os_linux.cpp 2018-12-20 23:34:55.898000195 -0500 @@ -705,6 +705,8 @@ } } + assert(osthread->pthread_id() != 0, "pthread_id was not set as expected"); + // call one more level start routine thread->call_run(); --- old/src/hotspot/share/gc/parallel/gcTaskThread.cpp 2018-12-20 23:35:03.714446824 -0500 +++ new/src/hotspot/share/gc/parallel/gcTaskThread.cpp 2018-12-20 23:35:01.894342824 -0500 @@ -114,7 +114,6 @@ // for tasks to be enqueued for execution. void GCTaskThread::run() { - this->initialize_named_thread(); // Bind yourself to your processor. if (processor_id() != GCTaskManager::sentinel_worker()) { log_trace(gc, task, thread)("GCTaskThread::run: binding to processor %u", processor_id()); --- old/src/hotspot/share/gc/shared/concurrentGCThread.cpp 2018-12-20 23:35:09.270764311 -0500 +++ new/src/hotspot/share/gc/shared/concurrentGCThread.cpp 2018-12-20 23:35:07.434659397 -0500 @@ -49,7 +49,6 @@ } void ConcurrentGCThread::initialize_in_thread() { - this->initialize_named_thread(); this->set_active_handles(JNIHandleBlock::allocate_block()); // From this time Thread::current() should be working. assert(this == Thread::current(), "just checking"); --- old/src/hotspot/share/gc/shared/workgroup.cpp 2018-12-20 23:35:14.935087966 -0500 +++ new/src/hotspot/share/gc/shared/workgroup.cpp 2018-12-20 23:35:13.194988539 -0500 @@ -297,7 +297,6 @@ } void AbstractGangWorker::initialize() { - this->initialize_named_thread(); assert(_gang != NULL, "No gang to run in"); os::set_priority(this, NearMaxPriority); log_develop_trace(gc, workgang)("Running gang worker for gang %s id %u", gang()->name(), id()); --- old/src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp 2018-12-20 23:35:20.471404304 -0500 +++ new/src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp 2018-12-20 23:35:18.579296195 -0500 @@ -335,7 +335,8 @@ void set_native_interval(size_t interval) { _interval_native = interval; }; size_t get_java_interval() { return _interval_java; }; size_t get_native_interval() { return _interval_native; }; - + protected: + virtual void post_run(); public: void run(); static Monitor* transition_block() { return JfrThreadSampler_lock; } @@ -484,6 +485,10 @@ last_native_ms = get_monotonic_ms(); } } +} + +void JfrThreadSampler::post_run() { + this->NonJavaThread::post_run(); delete this; } --- old/src/hotspot/share/runtime/thread.cpp 2018-12-20 23:35:25.867712648 -0500 +++ new/src/hotspot/share/runtime/thread.cpp 2018-12-20 23:35:24.131613448 -0500 @@ -213,8 +213,12 @@ // Base class for all threads: VMThread, WatcherThread, ConcurrentMarkSweepThread, // JavaThread +DEBUG_ONLY(Thread* Thread::_starting_thread = NULL;) Thread::Thread() { + + DEBUG_ONLY(_run_state = PRE_CALL_RUN;) + // stack and get_thread set_stack_base(NULL); set_stack_size(0); @@ -314,11 +318,11 @@ if (barrier_set != NULL) { barrier_set->on_thread_create(this); } else { -#ifdef ASSERT - static bool initial_thread_created = false; - assert(!initial_thread_created, "creating thread before barrier set"); - initial_thread_created = true; -#endif // ASSERT + // Only the main thread should be created before the barrier set + // and that happens just before Thread::current is set. No other thread + // can attach as the VM is not created yet, so they can't execute this code. + // If the main thread creates other threads before the barrier set that is an error. + assert(Thread::current_or_null() == NULL, "creating thread before barrier set"); } } @@ -368,9 +372,16 @@ #endif // INCLUDE_NMT void Thread::call_run() { + DEBUG_ONLY(_run_state = CALL_RUN;) + // At this point, Thread object should be fully initialized and // Thread::current() should be set. + assert(Thread::current_or_null() != NULL, "current thread is unset"); + assert(Thread::current_or_null() == this, "current thread is wrong"); + + // Perform common initialization actions + register_thread_stack_with_NMT(); JFR_ONLY(Jfr::on_thread_start(this);) @@ -380,25 +391,41 @@ os::current_thread_id(), p2i(stack_base() - stack_size()), p2i(stack_base()), stack_size()/1024); + // Perform initialization actions + DEBUG_ONLY(_run_state = PRE_RUN;) + this->pre_run(); + // Invoke ::run() + DEBUG_ONLY(_run_state = RUN;) this->run(); // Returned from ::run(). Thread finished. - // Note: at this point the thread object may already have deleted itself. - // So from here on do not dereference *this*. + // Perform common tear-down actions - // 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() == this, "current thread is wrong"); - Thread::clear_thread_current(); - } + assert(Thread::current_or_null() != NULL, "current thread is unset"); + assert(Thread::current_or_null() == this, "current thread is wrong"); + + // Perform tear-down actions + DEBUG_ONLY(_run_state = POST_RUN;) + this->post_run(); + + // Note: at this point the thread object may already have deleted itself, + // so from here on do not dereference *this*. Not all thread types currently + // delete themselves when they terminate. But no thread should ever be deleted + // asynchronously with respect to its termination - that is what _run_state can + // be used to check. + assert(Thread::current_or_null() == NULL, "current thread still present"); } Thread::~Thread() { + + // Attached threads will remain in PRE_CALL_RUN, as will threads that don't actually + // get started due to errors etc. Any active thread should at least reach post_run + // before it is deleted (usually in post_run()). + assert(_run_state == PRE_CALL_RUN || + _run_state == POST_RUN, "Active Thread deleted before post_run()"); + // Notify the barrier set that a thread is being destroyed. Note that a barrier // set might not be available if we encountered errors during bootstrapping. BarrierSet* const barrier_set = BarrierSet::barrier_set(); @@ -445,9 +472,10 @@ // osthread() can be NULL, if creation of thread failed. if (osthread() != NULL) os::free_thread(osthread()); - // clear Thread::current if thread is deleting itself. + // Clear Thread::current if thread is deleting itself and it has not + // already been done. This must be done before the memory is deallocated. // Needed to ensure JNI correctly detects non-attached threads. - if (this == Thread::current()) { + if (this == Thread::current_or_null()) { Thread::clear_thread_current(); } @@ -1051,7 +1079,9 @@ } bool Thread::set_as_starting_thread() { + assert(_starting_thread == NULL, "already initialized"); // NOTE: this must be called inside the main thread. + DEBUG_ONLY(_starting_thread = this;) return os::create_main_thread((JavaThread*)this); } @@ -1254,27 +1284,48 @@ } NonJavaThread::NonJavaThread() : Thread(), _next(NULL) { - // Add this thread to _the_list. + assert(BarrierSet::barrier_set() != NULL, "NonJavaThread created too soon!"); +} + +NonJavaThread::~NonJavaThread() { } + +void NonJavaThread::add_to_the_list() { MutexLockerEx lock(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag); _next = _the_list._head; OrderAccess::release_store(&_the_list._head, this); } -NonJavaThread::~NonJavaThread() { - JFR_ONLY(Jfr::on_thread_exit(this);) - // Remove this thread from _the_list. +void NonJavaThread::remove_from_the_list() { MutexLockerEx lock(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag); NonJavaThread* volatile* p = &_the_list._head; for (NonJavaThread* t = *p; t != NULL; p = &t->_next, t = *p) { if (t == this) { *p = this->_next; - // Wait for any in-progress iterators. + // Wait for any in-progress iterators. Concurrent synchronize is + // not allowed, so do it while holding the list lock. _the_list._protect.synchronize(); break; } } } +void NonJavaThread::pre_run() { + assert(BarrierSet::barrier_set() != NULL, "invariant"); + add_to_the_list(); + + // This is slightly odd in that NamedThread is a subclass, but + // in fact name() is defined in Thread + assert(this->name() != NULL, "thread name was not set before it was started"); + this->set_native_thread_name(this->name()); +} + +void NonJavaThread::post_run() { + JFR_ONLY(Jfr::on_thread_exit(this);) + remove_from_the_list(); + // Ensure thread-local-storage is cleared before termination. + Thread::clear_thread_current(); +} + // NamedThread -- non-JavaThread subclasses with multiple // uniquely named instances should derive from this. NamedThread::NamedThread() : @@ -1301,10 +1352,6 @@ va_end(ap); } -void NamedThread::initialize_named_thread() { - set_native_thread_name(name()); -} - void NamedThread::print_on(outputStream* st) const { st->print("\"%s\" ", name()); Thread::print_on(st); @@ -1401,7 +1448,6 @@ void WatcherThread::run() { assert(this == watcher_thread(), "just checking"); - this->set_native_thread_name(this->name()); this->set_active_handles(JNIHandleBlock::allocate_block()); while (true) { assert(watcher_thread() == Thread::current(), "thread consistency check"); @@ -1757,19 +1803,30 @@ } -// The first routine called by a new Java thread +// First JavaThread specific code executed by a new Java thread. +void JavaThread::pre_run() { + // empty - see comments in run() +} + +// The main routine called by a new Java thread. This isn't overridden +// by subclasses, instead different subclasses define a different "entry_point" +// which defines the actual logic for that kind of thread. void JavaThread::run() { // initialize thread-local alloc buffer related fields this->initialize_tlab(); - // used to test validity of stack trace backs + // Used to test validity of stack trace backs. + // This can't be moved into pre_run() else we invalidate + // the requirement that thread_main_inner is lower on + // the stack. Consequently all the initialization logic + // stays here in run() rather than pre_run(). this->record_base_of_stack_pointer(); this->create_stack_guard_pages(); this->cache_global_variables(); - // Thread is now sufficient initialized to be handled by the safepoint code as being + // Thread is now sufficiently initialized to be handled by the safepoint code as being // in the VM. Change thread state from _thread_new to _thread_in_vm ThreadStateTransition::transition_and_fence(this, _thread_new, _thread_in_vm); @@ -1788,13 +1845,10 @@ } // We call another function to do the rest so we are sure that the stack addresses used - // from there will be lower than the stack base just computed + // from there will be lower than the stack base just computed. thread_main_inner(); - - // Note, thread is no longer valid at this point! } - void JavaThread::thread_main_inner() { assert(JavaThread::current() == this, "sanity check"); assert(this->threadObj() != NULL, "just checking"); @@ -1814,11 +1868,17 @@ DTRACE_THREAD_PROBE(stop, this); + // Cleanup is handled in post_run() +} + +// Shared teardown for all JavaThreads +void JavaThread::post_run() { this->exit(false); + // Defer deletion to here to ensure 'this' is still referenceable in call_run + // for any shared tear-down. this->smr_delete(); } - static void ensure_join(JavaThread* thread) { // We do not need to grab the Threads_lock, since we are operating on ourself. Handle threadObj(thread, thread->threadObj()); --- old/src/hotspot/share/runtime/thread.hpp 2018-12-20 23:35:31.448031502 -0500 +++ new/src/hotspot/share/runtime/thread.hpp 2018-12-20 23:35:29.635927960 -0500 @@ -109,6 +109,31 @@ // This means !t->is_Java_thread() iff t is a NonJavaThread, or t is // a partially constructed/destroyed Thread. +/* + Thread execution sequence and actions: + + All threads: + - thread_native_entry // per-OS native entry point + - stack initialization + - other OS-level initialization (signal masks etc) + - handshake with creating thread (if not started suspended) + - this->call_run() // common shared entry point + - shared common initialization + - this->pre_run() // virtual per-thread-type initialization + - this->run() // virtual per-thread-type "main" logic + - shared common tear-down + - this->post_run() // virtual per-thread-type tear-down + - // 'this' no longer referenceable + - OS-level tear-down (minimal) + - final logging + + For JavaThread: + - this->run() // virtual but not normally overridden + - this->thread_main_inner() // extra call level to ensure correct stack calculations + - this->entry_point() // set differently for each kind of JavaThread + +*/ + class Thread: public ThreadShadow { friend class VMStructs; friend class JVMCIVMStructs; @@ -119,7 +144,6 @@ static THREAD_LOCAL_DECL Thread* _thr_current; #endif - private: // Thread local data area available to the GC. The internal // structure and contents of this data area is GC-specific. // Only GC and GC barrier code should access this data area. @@ -141,6 +165,9 @@ // const char* _exception_file; // file information for exception (debugging only) // int _exception_line; // line information for exception (debugging only) protected: + + DEBUG_ONLY(static Thread* _starting_thread;) + // Support for forcing alignment of thread objects for biased locking void* _real_malloc_address; @@ -417,6 +444,21 @@ protected: // To be implemented by children. virtual void run() = 0; + virtual void pre_run() = 0; + virtual void post_run() = 0; // Note: Thread must not be deleted prior to calling this! + +#ifdef ASSERT + enum RunState { + PRE_CALL_RUN, + CALL_RUN, + PRE_RUN, + RUN, + POST_RUN + // POST_CALL_RUN - can't define this one as 'this' may be deleted when we want to set it + }; + RunState _run_state; // for lifecycle checks +#endif + public: // invokes ::run(), with common preparations and cleanups. @@ -795,6 +837,13 @@ class List; static List _the_list; + void add_to_the_list(); + void remove_from_the_list(); + + protected: + virtual void pre_run(); + virtual void post_run(); + public: NonJavaThread(); ~NonJavaThread(); @@ -802,10 +851,19 @@ class Iterator; }; -// Provides iteration over the list of NonJavaThreads. Because list -// management occurs in the NonJavaThread constructor and destructor, -// entries in the list may not be fully constructed instances of a -// derived class. Threads created after an iterator is constructed +// Provides iteration over the list of NonJavaThreads. +// Normal list addition occurs in pre_run(), and removal occurs +// in post_run(), so that only live fully-initialized threads can +// be found in the list. There is a special case during VM startup +// when the BarrierSet has not yet been created, where we add to the +// list during the constructor. That set of initial threads (the main +// thread, plus GC threads typically) is iterated over when the +// BarrierSet is set and each thread is manually updated. The fact those +// threads may not yet have executed is not an issue for that code. +// However it is possible that those threads may still not have executed +// by the time subsequent iterators are created and used - but they are +// known to be fully initialized by their creating thread. +// Threads created after an iterator is constructed // will not be visited by the iterator. The scope of an iterator is a // critical section; there must be no safepoint checks in that scope. class NonJavaThread::Iterator : public StackObj { @@ -843,7 +901,6 @@ ~NamedThread(); // May only be called once per thread. void set_name(const char* format, ...) ATTRIBUTE_PRINTF(2, 3); - void initialize_named_thread(); virtual bool is_Named_thread() const { return true; } virtual char* name() const { return _name == NULL ? (char*)"Unknown Thread" : _name; } JavaThread *processed_thread() { return _processed_thread; } @@ -874,7 +931,7 @@ // A single WatcherThread is used for simulating timer interrupts. class WatcherThread: public NonJavaThread { friend class VMStructs; - public: + protected: virtual void run(); private: @@ -1831,9 +1888,9 @@ void print_name_on_error(outputStream* st, char* buf, int buflen) const; void verify(); const char* get_thread_name() const; - private: + protected: // factor out low-level mechanics for use in both normal and error cases - const char* get_thread_name_string(char* buf = NULL, int buflen = 0) const; + virtual const char* get_thread_name_string(char* buf = NULL, int buflen = 0) const; public: const char* get_threadgroup_name() const; const char* get_parent_name() const; @@ -1886,9 +1943,12 @@ inline CompilerThread* as_CompilerThread(); - public: + protected: + virtual void pre_run(); virtual void run(); void thread_main_inner(); + virtual void post_run(); + private: GrowableArray* _array_for_gc; --- old/src/hotspot/share/runtime/vmThread.cpp 2018-12-20 23:35:37.452374584 -0500 +++ new/src/hotspot/share/runtime/vmThread.cpp 2018-12-20 23:35:35.724275842 -0500 @@ -285,8 +285,6 @@ void VMThread::run() { assert(this == vm_thread(), "check"); - this->initialize_named_thread(); - // Notify_lock wait checks on active_handles() to rewait in // case of spurious wakeup, it should wait on the last // value set prior to the notify --- old/src/hotspot/share/services/management.cpp 2018-12-20 23:35:42.856683381 -0500 +++ new/src/hotspot/share/services/management.cpp 2018-12-20 23:35:41.120584182 -0500 @@ -1645,12 +1645,6 @@ return; } - // NonJavaThread instances may not be fully initialized yet, so we need to - // skip any that aren't - check for zero stack_size() - if (!thread->is_Java_thread() && thread->stack_size() == 0) { - return; - } - if (_count >= _names_len || _count >= _times_len) { // skip if the result array is not big enough return; --- old/test/hotspot/gtest/threadHelper.inline.hpp 2018-12-20 23:35:48.721018462 -0500 +++ new/test/hotspot/gtest/threadHelper.inline.hpp 2018-12-20 23:35:46.704903263 -0500 @@ -48,8 +48,11 @@ public: Semaphore _ready; Semaphore _unblock; - VMThreadBlocker() {} + VMThreadBlocker() { } virtual ~VMThreadBlocker() {} + const char* get_thread_name_string(char* buf, int buflen) const { + return (char*) "VMThreadBlocker"; + } void run() { this->set_thread_state(_thread_in_vm); { @@ -58,9 +61,15 @@ } VM_StopSafepoint ss(&_ready, &_unblock); VMThread::execute(&ss); + } + + // Override as JavaThread::post_run() calls JavaThread::exit which + // expects a valid thread object oop. + virtual void post_run() { Threads::remove(this); this->smr_delete(); } + void doit() { if (os::create_thread(this, os::os_thread)) { os::start_thread(this); @@ -85,7 +94,11 @@ } virtual ~JavaTestThread() {} - void prerun() { + const char* get_thread_name_string(char* buf, int buflen) const { + return (char*) "JavaTestThread"; + } + + void pre_run() { this->set_thread_state(_thread_in_vm); { MutexLocker ml(Threads_lock); @@ -99,12 +112,12 @@ virtual void main_run() = 0; void run() { - prerun(); main_run(); - postrun(); } - void postrun() { + // Override as JavaThread::post_run() calls JavaThread::exit which + // expects a valid thread object oop. And we need to call signal. + void post_run() { Threads::remove(this); _post->signal(); this->smr_delete();