8348268: Test gc/shenandoah/TestResizeTLAB.java#compact: fatal error: Before Updating References: Thread C2 CompilerThread1: expected gc-state 9, actual 21

Reviewed-by: shade

Author: William Kemper

src/hotspot/share/gc/shenandoah/shenandoahBarrierSet.cpp

@@ -113,8 +113,10 @@ void ShenandoahBarrierSet::on_thread_attach(Thread *thread) {
   assert(queue.buffer() == nullptr, "SATB queue should not have a buffer");
   assert(queue.index() == 0, "SATB queue index should be zero");
   queue.set_active(_satb_mark_queue_set.is_active());
+
+  ShenandoahThreadLocalData::set_gc_state(thread, _heap->gc_state());
+
   if (thread->is_Java_thread()) {
-    ShenandoahThreadLocalData::set_gc_state(thread, _heap->gc_state());
     ShenandoahThreadLocalData::initialize_gclab(thread);
 
     BarrierSetNMethod* bs_nm = barrier_set_nmethod();

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

@@ -1262,11 +1262,18 @@ void ShenandoahHeap::evacuate_collection_set(bool concurrent) {
 }
 
 void ShenandoahHeap::concurrent_prepare_for_update_refs() {
-  // It's possible that evacuation succeeded, but we could still be cancelled when we get here.
-  // A cancellation at this point means the degenerated cycle must resume from update-refs.
-  set_gc_state_concurrent(EVACUATION, false);
-  set_gc_state_concurrent(WEAK_ROOTS, false);
-  set_gc_state_concurrent(UPDATE_REFS, true);
+  {
+    // Java threads take this lock while they are being attached and added to the list of thread.
+    // If another thread holds this lock before we update the gc state, it will receive a stale
+    // gc state, but they will have been added to the list of java threads and so will be corrected
+    // by the following handshake.
+    MutexLocker lock(Threads_lock);
+
+    // A cancellation at this point means the degenerated cycle must resume from update-refs.
+    set_gc_state_concurrent(EVACUATION, false);
+    set_gc_state_concurrent(WEAK_ROOTS, false);
+    set_gc_state_concurrent(UPDATE_REFS, true);
+  }
 
   // This will propagate the gc state and retire gclabs and plabs for threads that require it.
   ShenandoahPrepareForUpdateRefs prepare_for_update_refs(_gc_state.raw_value());
@@ -2025,6 +2032,12 @@ void ShenandoahHeap::set_gc_state_at_safepoint(uint mask, bool value) {
 }
 
 void ShenandoahHeap::set_gc_state_concurrent(uint mask, bool value) {
+  // Holding the thread lock here assures that any thread created after we change the gc
+  // state will have the correct state. It also prevents attaching threads from seeing
+  // an inconsistent state. See ShenandoahBarrierSet::on_thread_attach for reference. Established
+  // threads will use their thread local copy of the gc state (changed by a handshake, or on a
+  // safepoint).
+  assert(Threads_lock->is_locked(), "Must hold thread lock for concurrent gc state change");
   _gc_state.set_cond(mask, value);
 }