From e61bcd09163a49a03a9d82faa4f3e89b71dc420f Mon Sep 17 00:00:00 2001 From: fyang Date: Sat, 24 Oct 2015 15:44:08 -0700 Subject: [PATCH] 8047212: runtime/ParallelClassLoading/bootstrap/random/inner-complex assert(ObjectSynchronizer::verify_objmon_isinpool(inf)) failed: monitor is invalid Summary: Fix race between ObjectMonitor alloc and verification code; teach SA about "static pointer volatile" fields. Reviewed-by: andrew --- src/share/vm/runtime/synchronizer.cpp | 94 +++++++++++++++------------ src/share/vm/runtime/synchronizer.hpp | 2 +- src/share/vm/runtime/vmStructs.cpp | 19 +++++- 3 files changed, 69 insertions(+), 46 deletions(-) diff --git a/src/share/vm/runtime/synchronizer.cpp b/src/share/vm/runtime/synchronizer.cpp index 6bd92333c..48bf3c51c 100644 --- a/src/share/vm/runtime/synchronizer.cpp +++ b/src/share/vm/runtime/synchronizer.cpp @@ -149,7 +149,7 @@ int dtrace_waited_probe(ObjectMonitor* monitor, Handle obj, Thread* thr) { #define NINFLATIONLOCKS 256 static volatile intptr_t InflationLocks [NINFLATIONLOCKS] ; -ObjectMonitor * ObjectSynchronizer::gBlockList = NULL ; +ObjectMonitor * volatile ObjectSynchronizer::gBlockList = NULL; ObjectMonitor * volatile ObjectSynchronizer::gFreeList = NULL ; ObjectMonitor * volatile ObjectSynchronizer::gOmInUseList = NULL ; int ObjectSynchronizer::gOmInUseCount = 0; @@ -830,18 +830,18 @@ JavaThread* ObjectSynchronizer::get_lock_owner(Handle h_obj, bool doLock) { // Visitors ... void ObjectSynchronizer::monitors_iterate(MonitorClosure* closure) { - ObjectMonitor* block = gBlockList; - ObjectMonitor* mid; - while (block) { + ObjectMonitor* block = + (ObjectMonitor*)OrderAccess::load_ptr_acquire(&gBlockList); + while (block != NULL) { assert(block->object() == CHAINMARKER, "must be a block header"); for (int i = _BLOCKSIZE - 1; i > 0; i--) { - mid = block + i; - oop object = (oop) mid->object(); + ObjectMonitor* mid = (ObjectMonitor *)(block + i); + oop object = (oop)mid->object(); if (object != NULL) { closure->do_monitor(mid); } } - block = (ObjectMonitor*) block->FreeNext; + block = (ObjectMonitor*)block->FreeNext; } } @@ -856,7 +856,9 @@ static inline ObjectMonitor* next(ObjectMonitor* block) { void ObjectSynchronizer::oops_do(OopClosure* f) { assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint"); - for (ObjectMonitor* block = gBlockList; block != NULL; block = next(block)) { + ObjectMonitor* block = + (ObjectMonitor*)OrderAccess::load_ptr_acquire(&gBlockList); + for (; block != NULL; block = (ObjectMonitor *)next(block)) { assert(block->object() == CHAINMARKER, "must be a block header"); for (int i = 1; i < _BLOCKSIZE; i++) { ObjectMonitor* mid = &block[i]; @@ -1059,7 +1061,9 @@ ObjectMonitor * ATTR ObjectSynchronizer::omAlloc (Thread * Self) { // The very first objectMonitor in a block is reserved and dedicated. // It serves as blocklist "next" linkage. temp[0].FreeNext = gBlockList; - gBlockList = temp; + // There are lock-free uses of gBlockList so make sure that + // the previous stores happen before we update gBlockList. + OrderAccess::release_store_ptr(&gBlockList, temp); // Add the new string of objectMonitors to the global free list temp[_BLOCKSIZE - 1].FreeNext = gFreeList ; @@ -1536,29 +1540,33 @@ void ObjectSynchronizer::deflate_idle_monitors() { nInuse += gOmInUseCount; } - } else for (ObjectMonitor* block = gBlockList; block != NULL; block = next(block)) { - // Iterate over all extant monitors - Scavenge all idle monitors. - assert(block->object() == CHAINMARKER, "must be a block header"); - nInCirculation += _BLOCKSIZE ; - for (int i = 1 ; i < _BLOCKSIZE; i++) { - ObjectMonitor* mid = &block[i]; - oop obj = (oop) mid->object(); - - if (obj == NULL) { - // The monitor is not associated with an object. - // The monitor should either be a thread-specific private - // free list or the global free list. - // obj == NULL IMPLIES mid->is_busy() == 0 - guarantee (!mid->is_busy(), "invariant") ; - continue ; - } - deflated = deflate_monitor(mid, obj, &FreeHead, &FreeTail); + } else { + ObjectMonitor* block = + (ObjectMonitor*)OrderAccess::load_ptr_acquire(&gBlockList); + for (; block != NULL; block = (ObjectMonitor*)next(block)) { + // Iterate over all extant monitors - Scavenge all idle monitors. + assert(block->object() == CHAINMARKER, "must be a block header"); + nInCirculation += _BLOCKSIZE; + for (int i = 1; i < _BLOCKSIZE; i++) { + ObjectMonitor* mid = (ObjectMonitor*)&block[i]; + oop obj = (oop)mid->object(); + + if (obj == NULL) { + // The monitor is not associated with an object. + // The monitor should either be a thread-specific private + // free list or the global free list. + // obj == NULL IMPLIES mid->is_busy() == 0 + guarantee(!mid->is_busy(), "invariant"); + continue; + } + deflated = deflate_monitor(mid, obj, &FreeHead, &FreeTail); - if (deflated) { - mid->FreeNext = NULL ; - nScavenged ++ ; - } else { - nInuse ++; + if (deflated) { + mid->FreeNext = NULL; + nScavenged++; + } else { + nInuse++; + } } } } @@ -1693,13 +1701,13 @@ void ObjectSynchronizer::sanity_checks(const bool verbose, // Verify all monitors in the monitor cache, the verification is weak. void ObjectSynchronizer::verify() { - ObjectMonitor* block = gBlockList; - ObjectMonitor* mid; - while (block) { + ObjectMonitor* block = + (ObjectMonitor *)OrderAccess::load_ptr_acquire(&gBlockList); + while (block != NULL) { assert(block->object() == CHAINMARKER, "must be a block header"); for (int i = 1; i < _BLOCKSIZE; i++) { - mid = block + i; - oop object = (oop) mid->object(); + ObjectMonitor* mid = (ObjectMonitor *)(block + i); + oop object = (oop)mid->object(); if (object != NULL) { mid->verify(); } @@ -1713,18 +1721,18 @@ void ObjectSynchronizer::verify() { // the list of extant blocks without taking a lock. int ObjectSynchronizer::verify_objmon_isinpool(ObjectMonitor *monitor) { - ObjectMonitor* block = gBlockList; - - while (block) { + ObjectMonitor* block = + (ObjectMonitor*)OrderAccess::load_ptr_acquire(&gBlockList); + while (block != NULL) { assert(block->object() == CHAINMARKER, "must be a block header"); if (monitor > &block[0] && monitor < &block[_BLOCKSIZE]) { - address mon = (address) monitor; - address blk = (address) block; + address mon = (address)monitor; + address blk = (address)block; size_t diff = mon - blk; - assert((diff % sizeof(ObjectMonitor)) == 0, "check"); + assert((diff % sizeof(ObjectMonitor)) == 0, "must be aligned"); return 1; } - block = (ObjectMonitor*) block->FreeNext; + block = (ObjectMonitor*)block->FreeNext; } return 0; } diff --git a/src/share/vm/runtime/synchronizer.hpp b/src/share/vm/runtime/synchronizer.hpp index 1bd635a0a..2dc220559 100644 --- a/src/share/vm/runtime/synchronizer.hpp +++ b/src/share/vm/runtime/synchronizer.hpp @@ -131,7 +131,7 @@ class ObjectSynchronizer : AllStatic { private: enum { _BLOCKSIZE = 128 }; - static ObjectMonitor* gBlockList; + static ObjectMonitor * volatile gBlockList; static ObjectMonitor * volatile gFreeList; static ObjectMonitor * volatile gOmInUseList; // for moribund thread, so monitors they inflated still get scanned static int gOmInUseCount; diff --git a/src/share/vm/runtime/vmStructs.cpp b/src/share/vm/runtime/vmStructs.cpp index 7f6f84887..42c5e5ef9 100644 --- a/src/share/vm/runtime/vmStructs.cpp +++ b/src/share/vm/runtime/vmStructs.cpp @@ -257,6 +257,7 @@ typedef TwoOopHashtable SymbolTwoOopHashtable; #define VM_STRUCTS(nonstatic_field, \ static_field, \ + static_ptr_volatile_field, \ unchecked_nonstatic_field, \ volatile_nonstatic_field, \ nonproduct_nonstatic_field, \ @@ -1082,7 +1083,7 @@ typedef TwoOopHashtable SymbolTwoOopHashtable; volatile_nonstatic_field(BasicLock, _displaced_header, markOop) \ nonstatic_field(BasicObjectLock, _lock, BasicLock) \ nonstatic_field(BasicObjectLock, _obj, oop) \ - static_field(ObjectSynchronizer, gBlockList, ObjectMonitor*) \ + static_ptr_volatile_field(ObjectSynchronizer,gBlockList, ObjectMonitor*) \ \ /*********************/ \ /* Matcher (C2 only) */ \ @@ -2667,6 +2668,11 @@ typedef TwoOopHashtable SymbolTwoOopHashtable; #define GENERATE_STATIC_VM_STRUCT_ENTRY(typeName, fieldName, type) \ { QUOTE(typeName), QUOTE(fieldName), QUOTE(type), 1, 0, &typeName::fieldName }, +// This macro generates a VMStructEntry line for a static pointer volatile field, +// e.g.: "static ObjectMonitor * volatile gBlockList;" +#define GENERATE_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY(typeName, fieldName, type) \ + { QUOTE(typeName), QUOTE(fieldName), QUOTE(type), 1, 0, (void*)&typeName::fieldName }, + // This macro generates a VMStructEntry line for an unchecked // nonstatic field, in which the size of the type is also specified. // The type string is given as NULL, indicating an "opaque" type. @@ -2692,10 +2698,15 @@ typedef TwoOopHashtable SymbolTwoOopHashtable; #define CHECK_VOLATILE_NONSTATIC_VM_STRUCT_ENTRY(typeName, fieldName, type) \ {typedef type dummyvtype; typeName *dummyObj = NULL; volatile dummyvtype* dummy = &dummyObj->fieldName; } -// This macro checks the type of a VMStructEntry by comparing pointer types +// This macro checks the type of a static VMStructEntry by comparing pointer types #define CHECK_STATIC_VM_STRUCT_ENTRY(typeName, fieldName, type) \ {type* dummy = &typeName::fieldName; } +// This macro checks the type of a static pointer volatile VMStructEntry by comparing pointer types, +// e.g.: "static ObjectMonitor * volatile gBlockList;" +#define CHECK_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY(typeName, fieldName, type) \ + {type volatile * dummy = &typeName::fieldName; } + // This macro ensures the type of a field and its containing type are // present in the type table. The assertion string is shorter than // preferable because (incredibly) of a bug in Solstice NFS client @@ -2889,6 +2900,7 @@ VMStructEntry VMStructs::localHotSpotVMStructs[] = { VM_STRUCTS(GENERATE_NONSTATIC_VM_STRUCT_ENTRY, GENERATE_STATIC_VM_STRUCT_ENTRY, + GENERATE_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY, GENERATE_UNCHECKED_NONSTATIC_VM_STRUCT_ENTRY, GENERATE_NONSTATIC_VM_STRUCT_ENTRY, GENERATE_NONPRODUCT_NONSTATIC_VM_STRUCT_ENTRY, @@ -3047,6 +3059,7 @@ void VMStructs::init() { VM_STRUCTS(CHECK_NONSTATIC_VM_STRUCT_ENTRY, CHECK_STATIC_VM_STRUCT_ENTRY, + CHECK_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY, CHECK_NO_OP, CHECK_VOLATILE_NONSTATIC_VM_STRUCT_ENTRY, CHECK_NONPRODUCT_NONSTATIC_VM_STRUCT_ENTRY, @@ -3162,8 +3175,10 @@ VMStructs::init() { CHECK_NO_OP, CHECK_NO_OP, CHECK_NO_OP, + CHECK_NO_OP, CHECK_NO_OP)); debug_only(VM_STRUCTS(CHECK_NO_OP, + ENSURE_FIELD_TYPE_PRESENT, ENSURE_FIELD_TYPE_PRESENT, CHECK_NO_OP, ENSURE_FIELD_TYPE_PRESENT, -- GitLab