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: cvarming, dholmes, sspitsyn, coleenp
--- a/hotspot/src/share/vm/runtime/synchronizer.cpp Fri Oct 23 23:06:53 2015 +0200
+++ b/hotspot/src/share/vm/runtime/synchronizer.cpp Sat Oct 24 15:44:08 2015 -0700
@@ -116,7 +116,7 @@
// global list of blocks of monitors
// gBlockList is really PaddedEnd<ObjectMonitor> *, but we don't
// want to expose the PaddedEnd template more than necessary.
-ObjectMonitor * ObjectSynchronizer::gBlockList = NULL;
+ObjectMonitor * volatile ObjectSynchronizer::gBlockList = NULL;
// global monitor free list
ObjectMonitor * volatile ObjectSynchronizer::gFreeList = NULL;
// global monitor in-use list, for moribund threads,
@@ -890,21 +890,22 @@
return NULL;
}
+
// Visitors ...
void ObjectSynchronizer::monitors_iterate(MonitorClosure* closure) {
- PaddedEnd<ObjectMonitor> * block = (PaddedEnd<ObjectMonitor> *)gBlockList;
- ObjectMonitor* mid;
- while (block) {
+ PaddedEnd<ObjectMonitor> * block =
+ (PaddedEnd<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 = (ObjectMonitor *)(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 = (PaddedEnd<ObjectMonitor> *) block->FreeNext;
+ block = (PaddedEnd<ObjectMonitor> *)block->FreeNext;
}
}
@@ -919,9 +920,9 @@
void ObjectSynchronizer::oops_do(OopClosure* f) {
assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
- for (PaddedEnd<ObjectMonitor> * block =
- (PaddedEnd<ObjectMonitor> *)gBlockList; block != NULL;
- block = (PaddedEnd<ObjectMonitor> *)next(block)) {
+ PaddedEnd<ObjectMonitor> * block =
+ (PaddedEnd<ObjectMonitor> *)OrderAccess::load_ptr_acquire(&gBlockList);
+ for (; block != NULL; block = (PaddedEnd<ObjectMonitor> *)next(block)) {
assert(block->object() == CHAINMARKER, "must be a block header");
for (int i = 1; i < _BLOCKSIZE; i++) {
ObjectMonitor* mid = (ObjectMonitor *)&block[i];
@@ -1139,7 +1140,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;
@@ -1621,31 +1624,33 @@
nInuse += gOmInUseCount;
}
- } else for (PaddedEnd<ObjectMonitor> * block =
- (PaddedEnd<ObjectMonitor> *)gBlockList; block != NULL;
- block = (PaddedEnd<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();
+ } else {
+ PaddedEnd<ObjectMonitor> * block =
+ (PaddedEnd<ObjectMonitor> *)OrderAccess::load_ptr_acquire(&gBlockList);
+ for (; block != NULL; block = (PaddedEnd<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, &freeHeadp, &freeTailp);
+ 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, &freeHeadp, &freeTailp);
- if (deflated) {
- mid->FreeNext = NULL;
- nScavenged++;
- } else {
- nInuse++;
+ if (deflated) {
+ mid->FreeNext = NULL;
+ nScavenged++;
+ } else {
+ nInuse++;
+ }
}
}
}
@@ -1789,18 +1794,18 @@
// Verify all monitors in the monitor cache, the verification is weak.
void ObjectSynchronizer::verify() {
- PaddedEnd<ObjectMonitor> * block = (PaddedEnd<ObjectMonitor> *)gBlockList;
- ObjectMonitor* mid;
- while (block) {
+ PaddedEnd<ObjectMonitor> * block =
+ (PaddedEnd<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 = (ObjectMonitor *)(block + i);
- oop object = (oop) mid->object();
+ ObjectMonitor* mid = (ObjectMonitor *)(block + i);
+ oop object = (oop)mid->object();
if (object != NULL) {
mid->verify();
}
}
- block = (PaddedEnd<ObjectMonitor> *) block->FreeNext;
+ block = (PaddedEnd<ObjectMonitor> *)block->FreeNext;
}
}
@@ -1809,19 +1814,19 @@
// the list of extant blocks without taking a lock.
int ObjectSynchronizer::verify_objmon_isinpool(ObjectMonitor *monitor) {
- PaddedEnd<ObjectMonitor> * block = (PaddedEnd<ObjectMonitor> *)gBlockList;
-
- while (block) {
+ PaddedEnd<ObjectMonitor> * block =
+ (PaddedEnd<ObjectMonitor> *)OrderAccess::load_ptr_acquire(&gBlockList);
+ while (block != NULL) {
assert(block->object() == CHAINMARKER, "must be a block header");
if (monitor > (ObjectMonitor *)&block[0] &&
monitor < (ObjectMonitor *)&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(PaddedEnd<ObjectMonitor>)) == 0, "check");
+ assert((diff % sizeof(PaddedEnd<ObjectMonitor>)) == 0, "must be aligned");
return 1;
}
- block = (PaddedEnd<ObjectMonitor> *) block->FreeNext;
+ block = (PaddedEnd<ObjectMonitor> *)block->FreeNext;
}
return 0;
}
--- a/hotspot/src/share/vm/runtime/synchronizer.hpp Fri Oct 23 23:06:53 2015 +0200
+++ b/hotspot/src/share/vm/runtime/synchronizer.hpp Sat Oct 24 15:44:08 2015 -0700
@@ -140,7 +140,7 @@
// global list of blocks of monitors
// gBlockList is really PaddedEnd<ObjectMonitor> *, but we don't
// want to expose the PaddedEnd template more than necessary.
- static ObjectMonitor * gBlockList;
+ static ObjectMonitor * volatile gBlockList;
// global monitor free list
static ObjectMonitor * volatile gFreeList;
// global monitor in-use list, for moribund threads,
--- a/hotspot/src/share/vm/runtime/vmStructs.cpp Fri Oct 23 23:06:53 2015 +0200
+++ b/hotspot/src/share/vm/runtime/vmStructs.cpp Sat Oct 24 15:44:08 2015 -0700
@@ -284,6 +284,7 @@
#define VM_STRUCTS(nonstatic_field, \
static_field, \
+ static_ptr_volatile_field, \
unchecked_nonstatic_field, \
volatile_nonstatic_field, \
nonproduct_nonstatic_field, \
@@ -1183,7 +1184,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) */ \
@@ -2902,6 +2903,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.
@@ -2927,10 +2933,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
@@ -3141,6 +3152,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,
@@ -3370,6 +3382,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,
@@ -3491,9 +3504,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,