# HG changeset patch # User fyang # Date 1445726648 25200 # Node ID ff1c3c1867b502c289c7c08ebc712663ec657f06 # Parent e517ff39c40d840792e3041df8d989ed2b761d85 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 diff -r e517ff39c40d -r ff1c3c1867b5 src/share/vm/runtime/synchronizer.cpp --- a/src/share/vm/runtime/synchronizer.cpp Wed Nov 04 10:12:37 2015 -0800 +++ b/src/share/vm/runtime/synchronizer.cpp Sat Oct 24 15:44:08 2015 -0700 @@ -149,7 +149,7 @@ #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 @@ // 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 @@ 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 @@ // 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 @@ 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(); + } 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 (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 @@ // 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 @@ // 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 -r e517ff39c40d -r ff1c3c1867b5 src/share/vm/runtime/synchronizer.hpp --- a/src/share/vm/runtime/synchronizer.hpp Wed Nov 04 10:12:37 2015 -0800 +++ b/src/share/vm/runtime/synchronizer.hpp Sat Oct 24 15:44:08 2015 -0700 @@ -131,7 +131,7 @@ 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 -r e517ff39c40d -r ff1c3c1867b5 src/share/vm/runtime/vmStructs.cpp --- a/src/share/vm/runtime/vmStructs.cpp Wed Nov 04 10:12:37 2015 -0800 +++ b/src/share/vm/runtime/vmStructs.cpp Sat Oct 24 15:44:08 2015 -0700 @@ -257,6 +257,7 @@ #define VM_STRUCTS(nonstatic_field, \ static_field, \ + static_ptr_volatile_field, \ unchecked_nonstatic_field, \ volatile_nonstatic_field, \ nonproduct_nonstatic_field, \ @@ -1082,7 +1083,7 @@ 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 @@ #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 @@ #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 @@ 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 @@ 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,9 +3175,11 @@ 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, ENSURE_NONPRODUCT_FIELD_TYPE_PRESENT,