Fix the jweak implementation.
We need to distinguish between "cleared weak global" and "deleted weak global".
Previously we used NULL for both. Now we add a magic value for cleared weak
globals. I've also switched the GC over to using iterators, so IndirectRefTable
itself becomes responsible for not showing bad pointers to the GC.
I've also improved the reference table dumping to cope with the new scheme and
to be a bit easier to read (through extra indentation).
Bug: 4260055
Change-Id: I26af301fb2b46d014c6f6b0915a8f8a7fb6d7c5b
diff --git a/vm/CheckJni.cpp b/vm/CheckJni.cpp
index 22ecce7..a09859b 100644
--- a/vm/CheckJni.cpp
+++ b/vm/CheckJni.cpp
@@ -749,14 +749,16 @@
printWarn = true;
} else {
Object* obj = dvmDecodeIndirectRef(mEnv, jobj);
-
- /*
- * The decoded object will be NULL if this is a weak global ref
- * with a cleared referent.
- */
- if (obj == kInvalidIndirectRefObject || (obj != NULL && !dvmIsValidObject(obj))) {
- LOGW("JNI WARNING: native code passing in bad object %p %p", jobj, obj);
- printWarn = true;
+ if (obj != NULL) {
+ if (obj == kInvalidIndirectRefObject) {
+ LOGW("JNI WARNING: native code passing in invalid reference %p", jobj);
+ printWarn = true;
+ } else if (obj != kClearedJniWeakGlobal && !dvmIsValidObject(obj)) {
+ // TODO: when we remove workAroundAppJniBugs, this should be impossible.
+ LOGW("JNI WARNING: native code passing in reference to invalid object %p %p",
+ jobj, obj);
+ printWarn = true;
+ }
}
}
diff --git a/vm/IndirectRefTable.cpp b/vm/IndirectRefTable.cpp
index 6a50dab..58fa0f7 100644
--- a/vm/IndirectRefTable.cpp
+++ b/vm/IndirectRefTable.cpp
@@ -75,11 +75,9 @@
Object* obj = table[idx];
IndirectRef checkRef = toIndirectRef(obj, idx);
if (checkRef != iref) {
- if (indirectRefKind(iref) != kIndirectKindWeakGlobal) {
- LOGE("JNI ERROR (app bug): attempt to %s stale %s reference (req=%p vs cur=%p; table=%p)",
- what, indirectRefKindToString(kind), iref, checkRef, this);
- abortMaybe();
- }
+ LOGE("JNI ERROR (app bug): attempt to %s stale %s reference %p (should be %p)",
+ what, indirectRefKindToString(kind), iref, checkRef);
+ abortMaybe();
return false;
}
return true;
@@ -112,27 +110,23 @@
}
assert(newSize > allocEntries);
- Object** newTable = (Object**) realloc(table, newSize * sizeof(Object*));
- if (newTable == NULL) {
+ table = (Object**) realloc(table, newSize * sizeof(Object*));
+ if (table == NULL) {
LOGE("JNI ERROR (app bug): unable to expand %s reference table (from %d to %d, max=%d)",
indirectRefKindToString(kind),
allocEntries, newSize, maxEntries);
dump(indirectRefKindToString(kind));
dvmAbort();
}
-
- /* update entries; adjust "nextEntry" in case memory moved */
- table = newTable;
allocEntries = newSize;
}
- IndirectRef result;
-
/*
* We know there's enough room in the table. Now we just need to find
* the right spot. If there's a hole, find it and fill it; otherwise,
* add to the end of the list.
*/
+ IndirectRef result;
int numHoles = segmentState.parts.numHoles - prevState.parts.numHoles;
if (numHoles > 0) {
assert(topIndex > 1);
@@ -166,12 +160,11 @@
bool IndirectRefTable::getChecked(IndirectRef iref) const
{
if (iref == NULL) {
- LOGW("Attempt to look up NULL %s reference",
- indirectRefKindToString(kind));
+ LOGW("Attempt to look up NULL %s reference", indirectRefKindToString(kind));
return false;
}
if (indirectRefKind(iref) == kIndirectKindInvalid) {
- LOGE("JNI ERROR (app bug): invalid %s reference (%p)",
+ LOGE("JNI ERROR (app bug): invalid %s reference %p",
indirectRefKindToString(kind), iref);
abortMaybe();
return false;
@@ -181,8 +174,15 @@
int idx = extractIndex(iref);
if (idx >= topIndex) {
/* bad -- stale reference? */
- LOGE("JNI ERROR (app bug): accessed stale %s reference at index %d (top=%d)",
- indirectRefKindToString(kind), idx, topIndex);
+ LOGE("JNI ERROR (app bug): accessed stale %s reference %p (index %d in a table of size %d)",
+ indirectRefKindToString(kind), iref, idx, topIndex);
+ abortMaybe();
+ return false;
+ }
+
+ if (table[idx] == NULL) {
+ LOGI("JNI ERROR (app bug): accessed deleted %s reference %p",
+ indirectRefKindToString(kind), iref);
abortMaybe();
return false;
}
@@ -232,11 +232,11 @@
assert(segmentState.parts.numHoles >= prevState.parts.numHoles);
int idx = extractIndex(iref);
- bool fakeDirectReferenceHack = false;
+ bool workAroundAppJniBugs = false;
if (indirectRefKind(iref) == kIndirectKindInvalid && gDvmJni.workAroundAppJniBugs) {
idx = linearScan(iref, bottomIndex, topIndex, table);
- fakeDirectReferenceHack = true;
+ workAroundAppJniBugs = true;
if (idx == -1) {
LOGW("trying to work around app JNI bugs, but didn't find %p in table!", iref);
return false;
@@ -257,31 +257,25 @@
}
if (idx == topIndex-1) {
- /*
- * Top-most entry. Scan up and consume holes. No need to NULL
- * out the entry, since the test vs. topIndex will catch it.
- */
- if (fakeDirectReferenceHack == false && !checkEntry("remove", iref, idx)) {
+ // Top-most entry. Scan up and consume holes.
+
+ if (workAroundAppJniBugs == false && !checkEntry("remove", iref, idx)) {
return false;
}
-#ifndef NDEBUG
- table[idx] = (Object*)0xd3d3d3d3;
-#endif
-
- int numHoles =
- segmentState.parts.numHoles - prevState.parts.numHoles;
+ table[idx] = NULL;
+ int numHoles = segmentState.parts.numHoles - prevState.parts.numHoles;
if (numHoles != 0) {
while (--topIndex > bottomIndex && numHoles != 0) {
LOGV("+++ checking for hole at %d (cookie=0x%08x) val=%p",
topIndex-1, cookie, table[topIndex-1]);
- if (table[topIndex-1] != NULL)
+ if (table[topIndex-1] != NULL) {
break;
+ }
LOGV("+++ ate hole at %d", topIndex-1);
numHoles--;
}
- segmentState.parts.numHoles =
- numHoles + prevState.parts.numHoles;
+ segmentState.parts.numHoles = numHoles + prevState.parts.numHoles;
segmentState.parts.topIndex = topIndex;
} else {
segmentState.parts.topIndex = topIndex-1;
@@ -297,14 +291,13 @@
LOGV("--- WEIRD: removing null entry %d", idx);
return false;
}
- if (fakeDirectReferenceHack == false && !checkEntry("remove", iref, idx)) {
+ if (workAroundAppJniBugs == false && !checkEntry("remove", iref, idx)) {
return false;
}
table[idx] = NULL;
segmentState.parts.numHoles++;
- LOGV("+++ left hole at %d, holes=%d",
- idx, segmentState.parts.numHoles);
+ LOGV("+++ left hole at %d, holes=%d", idx, segmentState.parts.numHoles);
}
return true;
diff --git a/vm/IndirectRefTable.h b/vm/IndirectRefTable.h
index 27e70f1..561ecd3 100644
--- a/vm/IndirectRefTable.h
+++ b/vm/IndirectRefTable.h
@@ -86,7 +86,9 @@
typedef void* IndirectRef;
/* magic failure value; must not pass dvmIsValidObject() */
-#define kInvalidIndirectRefObject ((Object*)0xdead4321)
+#define kInvalidIndirectRefObject reinterpret_cast<Object*>(0xdead4321)
+
+#define kClearedJniWeakGlobal reinterpret_cast<Object*>(0xdead1234)
/*
* Indirect reference kind, used as the two low bits of IndirectRef.
@@ -196,19 +198,62 @@
u4 numHoles:16; /* #of holes in entire table */
} parts;
};
+
+class iref_iterator {
+public:
+ explicit iref_iterator(Object** table, size_t i, size_t capacity)
+ : table_(table), i_(i), capacity_(capacity)
+ {
+ skipNullsAndTombstones();
+ }
+
+ iref_iterator& operator++() {
+ ++i_;
+ skipNullsAndTombstones();
+ return *this;
+ }
+
+ Object** operator*() {
+ return &table_[i_];
+ }
+
+ bool equals(const iref_iterator& rhs) const {
+ return (i_ == rhs.i_ && table_ == rhs.table_);
+ }
+
+ size_t to_i() const { return i_; }
+
+private:
+ void skipNullsAndTombstones() {
+ // We skip NULLs and tombstones. Clients don't want to see implementation details.
+ while (i_ < capacity_ && (table_[i_] == NULL || table_[i_] == kClearedJniWeakGlobal)) {
+ ++i_;
+ }
+ }
+
+ Object** table_;
+ size_t i_;
+ size_t capacity_;
+};
+
+bool inline operator!=(const iref_iterator& lhs, const iref_iterator& rhs) {
+ return !lhs.equals(rhs);
+}
+
struct IndirectRefTable {
+public:
+ typedef iref_iterator iterator;
+
/* semi-public - read/write by interpreter in native call handler */
IRTSegmentState segmentState;
- /* semi-public - read-only during GC scan; pointer must not be kept */
- Object** table; /* bottom of the stack */
-
/*
* private:
*
* TODO: we can't make these private as long as the interpreter
* uses offsetof, since private member data makes us non-POD.
*/
+ Object** table; /* bottom of the stack */
IndirectRefKind kind; /* bit mask, ORed into all irefs */
IndirectRefSlot* slotData; /* extended debugging info */
size_t allocEntries; /* #of entries we have space for */
@@ -292,6 +337,14 @@
return segmentState.parts.topIndex;
}
+ iterator begin() {
+ return iterator(table, 0, capacity());
+ }
+
+ iterator end() {
+ return iterator(table, capacity(), capacity());
+ }
+
private:
/*
* Extract the table index from an indirect reference.
diff --git a/vm/Jni.cpp b/vm/Jni.cpp
index 814a657..a9ac1cc 100644
--- a/vm/Jni.cpp
+++ b/vm/Jni.cpp
@@ -351,19 +351,11 @@
IndirectRefTable* pRefTable = &gDvm.jniWeakGlobalRefTable;
ScopedPthreadMutexLock lock(&gDvm.jniWeakGlobalRefLock);
Object* result = pRefTable->get(jobj);
- // There's no null check here because we might be dealing with
- // a cleared weak global reference.
- /*
- * TODO: this is a temporary workaround for broken weak global
- * refs (http://b/4260055). We treat any invalid reference as if it
- * were a weak global with a cleared referent. This means that
- * actual invalid references won't be detected, and if an empty
- * slot gets re-used we will return the new reference instead.
- * This must be removed when weak global refs get fixed.
- */
- if (result == kInvalidIndirectRefObject) {
- LOGW("Warning: used weak global ref hack");
+ if (result == kClearedJniWeakGlobal) {
result = NULL;
+ } else if (result == NULL) {
+ LOGE("JNI ERROR (app bug): use of deleted weak global reference (%p)", jobj);
+ dvmAbort();
}
return result;
}
@@ -566,9 +558,7 @@
IndirectRefTable *table = &gDvm.jniWeakGlobalRefTable;
jobject jobj = (jobject) table->add(IRT_FIRST_SEGMENT, obj);
if (jobj == NULL) {
- table->dump("JNI weak global");
- LOGE("Failed adding to JNI weak global ref table (%zd entries)",
- table->capacity());
+ LOGE("Failed adding to JNI weak global ref table (%zd entries)", table->capacity());
}
return jobj;
}
diff --git a/vm/ReferenceTable.cpp b/vm/ReferenceTable.cpp
index 65a38c8..4ec80ae 100644
--- a/vm/ReferenceTable.cpp
+++ b/vm/ReferenceTable.cpp
@@ -157,8 +157,10 @@
static size_t getElementCount(const Object* obj)
{
const ArrayObject* arrayObj = (ArrayObject*) obj;
- if (arrayObj == NULL || arrayObj->clazz == NULL || !dvmIsArray(arrayObj))
+ if (arrayObj == NULL || arrayObj == kClearedJniWeakGlobal ||
+ arrayObj->clazz == NULL || !dvmIsArray(arrayObj)) {
return 0;
+ }
return arrayObj->length;
}
@@ -171,7 +173,7 @@
const Object* obj1 = *((Object* const*) vobj1);
const Object* obj2 = *((Object* const*) vobj2);
- /* ensure null references appear at the end */
+ // Ensure null references and cleared jweaks appear at the end.
if (obj1 == NULL) {
if (obj2 == NULL) {
return 0;
@@ -181,6 +183,15 @@
} else if (obj2 == NULL) {
return -1;
}
+ if (obj1 == kClearedJniWeakGlobal) {
+ if (obj2 == kClearedJniWeakGlobal) {
+ return 0;
+ } else {
+ return 1;
+ }
+ } else if (obj2 == kClearedJniWeakGlobal) {
+ return -1;
+ }
if (obj1->clazz != obj2->clazz) {
return (u1*)obj1->clazz - (u1*)obj2->clazz;
@@ -205,7 +216,11 @@
static void logSummaryLine(const Object* obj, size_t elems, int identical, int equiv)
{
if (obj == NULL) {
- LOGW(" NULL reference (count=%d)", equiv);
+ LOGW(" NULL reference (count=%d)", equiv);
+ return;
+ }
+ if (obj == kClearedJniWeakGlobal) {
+ LOGW(" cleared jweak (count=%d)", equiv);
return;
}
@@ -224,7 +239,7 @@
if (identical + equiv != 0) {
StringAppendF(&msg, " (%d unique instances)", equiv + 1);
}
- LOGW("%s", msg.c_str());
+ LOGW(" %s", msg.c_str());
}
/*
@@ -236,30 +251,33 @@
void dvmDumpReferenceTableContents(Object* const* refs, size_t count,
const char* descr)
{
+ LOGW("%s reference table (%p) dump:", descr, refs);
+
if (count == 0) {
- LOGW("%s reference table has no entries", descr);
+ LOGW(" (empty)");
return;
}
- /*
- * Dump the most recent N entries.
- */
+ // Dump the most recent N entries.
const size_t kLast = 10;
- LOGW("Last %d entries in %s reference table:", kLast, descr);
int first = count - kLast;
if (first < 0) {
first = 0;
}
-
+ LOGW(" Last %d entries (of %d):", (count - first), count);
for (int idx = count - 1; idx >= first; --idx) {
const Object* ref = refs[idx];
if (ref == NULL) {
continue;
}
+ if (ref == kClearedJniWeakGlobal) {
+ LOGW(" %5d: cleared jweak", idx);
+ continue;
+ }
if (ref->clazz == NULL) {
- /* should only be possible right after a plain dvmMalloc() */
+ // should only be possible right after a plain dvmMalloc().
size_t size = dvmObjectSizeInHeap(ref);
- LOGW("%5d: %p (raw) (%zd bytes)", idx, ref, size);
+ LOGW(" %5d: %p (raw) (%zd bytes)", idx, ref, size);
continue;
}
@@ -286,12 +304,10 @@
}
free(s);
}
- LOGW("%5d: %p %s%s", idx, ref, className.c_str(), extras.c_str());
+ LOGW(" %5d: %p %s%s", idx, ref, className.c_str(), extras.c_str());
}
- /*
- * Make a copy of the table, and sort it.
- */
+ // Make a copy of the table, and sort it.
Object** tableCopy = (Object**)malloc(sizeof(Object*) * count);
if (tableCopy == NULL) {
LOGE("Unable to copy table with %d elements", count);
@@ -301,23 +317,19 @@
qsort(tableCopy, count, sizeof(Object*), compareObject);
refs = tableCopy; // use sorted list
- /*
- * Find and remove any "holes" in the list. The sort moved them all
- * to the end.
- *
- * A table with nothing but NULL entries should have count==0, which
- * was handled above, so this operation should not leave us with an
- * empty list.
- */
- while (refs[count-1] == NULL) {
- count--;
+ // Remove any uninteresting stuff from the list. The sort moved them all to the end.
+ while (count > 0 && refs[count-1] == NULL) {
+ --count;
}
- assert(count > 0);
+ while (count > 0 && refs[count-1] == kClearedJniWeakGlobal) {
+ --count;
+ }
+ if (count == 0) {
+ return;
+ }
- /*
- * Dump uniquified table summary.
- */
- LOGW("%s reference table summary (%d entries):", descr, count);
+ // Dump a summary of the whole table.
+ LOGW(" Summary:");
size_t equiv, identical;
equiv = identical = 0;
size_t idx;
@@ -326,21 +338,21 @@
elems = getElementCount(refs[idx-1]);
if (refs[idx] == refs[idx-1]) {
- /* same reference, added more than once */
+ // same reference, added more than once.
identical++;
} else if (refs[idx]->clazz == refs[idx-1]->clazz &&
getElementCount(refs[idx]) == elems)
{
- /* same class / element count, different object */
+ // same class / element count, different object.
equiv++;
} else {
- /* different class */
+ // different class.
logSummaryLine(refs[idx-1], elems, identical, equiv);
equiv = identical = 0;
}
}
- /* handle the last entry (everything above outputs refs[i-1]) */
+ // Handle the last entry (everything above outputs refs[i-1]).
elems = getElementCount(refs[idx-1]);
logSummaryLine(refs[count-1], elems, identical, equiv);
diff --git a/vm/alloc/MarkSweep.cpp b/vm/alloc/MarkSweep.cpp
index 19fadc1..3868909 100644
--- a/vm/alloc/MarkSweep.cpp
+++ b/vm/alloc/MarkSweep.cpp
@@ -996,15 +996,15 @@
return !isMarked((Object *)obj, &gDvm.gcHeap->markContext);
}
-void sweepWeakJniGlobals()
+static void sweepWeakJniGlobals()
{
- IndirectRefTable *table = &gDvm.jniWeakGlobalRefTable;
- Object **entry = table->table;
- GcMarkContext *ctx = &gDvm.gcHeap->markContext;
- int numEntries = table->capacity();
- for (int i = 0; i < numEntries; ++i) {
- if (entry[i] != NULL && !isMarked(entry[i], ctx)) {
- entry[i] = NULL;
+ IndirectRefTable* table = &gDvm.jniWeakGlobalRefTable;
+ GcMarkContext* ctx = &gDvm.gcHeap->markContext;
+ typedef IndirectRefTable::iterator It; // TODO: C++0x auto
+ for (It it = table->begin(), end = table->end(); it != end; ++it) {
+ Object** entry = *it;
+ if (!isMarked(*entry, ctx)) {
+ *entry = kClearedJniWeakGlobal;
}
}
}
diff --git a/vm/alloc/Visit.cpp b/vm/alloc/Visit.cpp
index a869418..7eaff07 100644
--- a/vm/alloc/Visit.cpp
+++ b/vm/alloc/Visit.cpp
@@ -70,10 +70,9 @@
{
assert(visitor != NULL);
assert(table != NULL);
- Object **entry = table->table;
- int numEntries = table->capacity();
- for (int i = 0; i < numEntries; ++i) {
- (*visitor)(&entry[i], threadId, type, arg);
+ typedef IndirectRefTable::iterator It; // TODO: C++0x auto
+ for (It it = table->begin(), end = table->end(); it != end; ++it) {
+ (*visitor)(*it, threadId, type, arg);
}
}
