Fix segv in indirect reference table dump.
ReferenceTable::Dump is common code but can't handle NULLs. Process
IndirectReferenceTable's use so that it doesn't pass NULLs for removed
slots.
Improve unit tests.
Change-Id: Ia41375f8f2ecc907ceda8c39d146ce6ef607fb08
diff --git a/src/indirect_reference_table.cc b/src/indirect_reference_table.cc
index 958531d..9bb6edc 100644
--- a/src/indirect_reference_table.cc
+++ b/src/indirect_reference_table.cc
@@ -318,6 +318,12 @@
void IndirectReferenceTable::Dump(std::ostream& os) const {
os << kind_ << " table dump:\n";
std::vector<const Object*> entries(table_, table_ + Capacity());
+ // Remove NULLs.
+ for (int i = entries.size() - 1; i >= 0; --i) {
+ if (entries[i] == NULL) {
+ entries.erase(entries.begin() + i);
+ }
+ }
ReferenceTable::Dump(os, entries);
}
diff --git a/src/indirect_reference_table_test.cc b/src/indirect_reference_table_test.cc
index 1698f18..971e42a 100644
--- a/src/indirect_reference_table_test.cc
+++ b/src/indirect_reference_table_test.cc
@@ -23,6 +23,24 @@
class IndirectReferenceTableTest : public CommonTest {
};
+static void CheckDump(IndirectReferenceTable* irt, size_t num_objects, size_t num_unique)
+ SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+ std::ostringstream oss;
+ irt->Dump(oss);
+ if (num_objects == 0) {
+ EXPECT_EQ(oss.str().find("java.lang.Object"), std::string::npos) << oss.str();
+ } else if (num_objects == 1) {
+ EXPECT_NE(oss.str().find("1 of java.lang.Object"), std::string::npos) << oss.str();
+ } else {
+ EXPECT_NE(oss.str().find(StringPrintf("%zd of java.lang.Object (%zd unique instances)",
+ num_objects, num_unique)),
+ std::string::npos)
+ << "\n Expected number of objects: " << num_objects
+ << "\n Expected unique objects: " << num_unique << "\n"
+ << oss.str();
+ }
+}
+
TEST_F(IndirectReferenceTableTest, BasicTest) {
ScopedObjectAccess soa(Thread::Current());
static const size_t kTableInitial = 10;
@@ -42,26 +60,32 @@
const uint32_t cookie = IRT_FIRST_SEGMENT;
+ CheckDump(&irt, 0, 0);
+
IndirectRef iref0 = (IndirectRef) 0x11110;
EXPECT_FALSE(irt.Remove(cookie, iref0)) << "unexpectedly successful removal";
// Add three, check, remove in the order in which they were added.
iref0 = irt.Add(cookie, obj0);
EXPECT_TRUE(iref0 != NULL);
+ CheckDump(&irt, 1, 1);
IndirectRef iref1 = irt.Add(cookie, obj1);
EXPECT_TRUE(iref1 != NULL);
+ CheckDump(&irt, 2, 2);
IndirectRef iref2 = irt.Add(cookie, obj2);
EXPECT_TRUE(iref2 != NULL);
-
- irt.Dump(LOG(DEBUG));
+ CheckDump(&irt, 3, 3);
EXPECT_EQ(obj0, irt.Get(iref0));
EXPECT_EQ(obj1, irt.Get(iref1));
EXPECT_EQ(obj2, irt.Get(iref2));
EXPECT_TRUE(irt.Remove(cookie, iref0));
+ CheckDump(&irt, 2, 2);
EXPECT_TRUE(irt.Remove(cookie, iref1));
+ CheckDump(&irt, 1, 1);
EXPECT_TRUE(irt.Remove(cookie, iref2));
+ CheckDump(&irt, 0, 0);
// Table should be empty now.
EXPECT_EQ(0U, irt.Capacity());
@@ -76,10 +100,14 @@
EXPECT_TRUE(iref1 != NULL);
iref2 = irt.Add(cookie, obj2);
EXPECT_TRUE(iref2 != NULL);
+ CheckDump(&irt, 3, 3);
ASSERT_TRUE(irt.Remove(cookie, iref2));
+ CheckDump(&irt, 2, 2);
ASSERT_TRUE(irt.Remove(cookie, iref1));
+ CheckDump(&irt, 1, 1);
ASSERT_TRUE(irt.Remove(cookie, iref0));
+ CheckDump(&irt, 0, 0);
// Table should be empty now.
ASSERT_EQ(0U, irt.Capacity());
@@ -92,17 +120,22 @@
EXPECT_TRUE(iref1 != NULL);
iref2 = irt.Add(cookie, obj2);
EXPECT_TRUE(iref2 != NULL);
+ CheckDump(&irt, 3, 3);
ASSERT_EQ(3U, irt.Capacity());
ASSERT_TRUE(irt.Remove(cookie, iref1));
+ CheckDump(&irt, 2, 2);
ASSERT_FALSE(irt.Remove(cookie, iref1));
+ CheckDump(&irt, 2, 2);
// Get invalid entry (from hole).
EXPECT_EQ(kInvalidIndirectRefObject, irt.Get(iref1));
ASSERT_TRUE(irt.Remove(cookie, iref2));
+ CheckDump(&irt, 1, 1);
ASSERT_TRUE(irt.Remove(cookie, iref0));
+ CheckDump(&irt, 0, 0);
// Table should be empty now.
ASSERT_EQ(0U, irt.Capacity());
@@ -118,21 +151,28 @@
EXPECT_TRUE(iref2 != NULL);
IndirectRef iref3 = irt.Add(cookie, obj3);
EXPECT_TRUE(iref3 != NULL);
+ CheckDump(&irt, 4, 4);
ASSERT_TRUE(irt.Remove(cookie, iref1));
+ CheckDump(&irt, 3, 3);
iref1 = irt.Add(cookie, obj1);
EXPECT_TRUE(iref1 != NULL);
ASSERT_EQ(4U, irt.Capacity()) << "hole not filled";
+ CheckDump(&irt, 4, 4);
ASSERT_TRUE(irt.Remove(cookie, iref1));
+ CheckDump(&irt, 3, 3);
ASSERT_TRUE(irt.Remove(cookie, iref3));
+ CheckDump(&irt, 2, 2);
ASSERT_EQ(3U, irt.Capacity()) << "should be 3 after two deletions";
ASSERT_TRUE(irt.Remove(cookie, iref2));
+ CheckDump(&irt, 1, 1);
ASSERT_TRUE(irt.Remove(cookie, iref0));
+ CheckDump(&irt, 0, 0);
ASSERT_EQ(0U, irt.Capacity()) << "not empty after split remove";
@@ -141,26 +181,35 @@
// With the extended checks in place, this should fail.
iref0 = irt.Add(cookie, obj0);
EXPECT_TRUE(iref0 != NULL);
+ CheckDump(&irt, 1, 1);
ASSERT_TRUE(irt.Remove(cookie, iref0));
+ CheckDump(&irt, 0, 0);
iref1 = irt.Add(cookie, obj1);
EXPECT_TRUE(iref1 != NULL);
+ CheckDump(&irt, 1, 1);
ASSERT_FALSE(irt.Remove(cookie, iref0)) << "mismatched del succeeded";
+ CheckDump(&irt, 1, 1);
ASSERT_TRUE(irt.Remove(cookie, iref1)) << "switched del failed";
ASSERT_EQ(0U, irt.Capacity()) << "switching del not empty";
+ CheckDump(&irt, 0, 0);
// Same as above, but with the same object. A more rigorous checker
// (e.g. with slot serialization) will catch this.
iref0 = irt.Add(cookie, obj0);
EXPECT_TRUE(iref0 != NULL);
+ CheckDump(&irt, 1, 1);
ASSERT_TRUE(irt.Remove(cookie, iref0));
+ CheckDump(&irt, 0, 0);
iref1 = irt.Add(cookie, obj0);
EXPECT_TRUE(iref1 != NULL);
+ CheckDump(&irt, 1, 1);
if (iref0 != iref1) {
// Try 0, should not work.
ASSERT_FALSE(irt.Remove(cookie, iref0)) << "temporal del succeeded";
}
ASSERT_TRUE(irt.Remove(cookie, iref1)) << "temporal cleanup failed";
ASSERT_EQ(0U, irt.Capacity()) << "temporal del not empty";
+ CheckDump(&irt, 0, 0);
// NULL isn't a valid iref.
ASSERT_EQ(kInvalidIndirectRefObject, irt.Get(NULL));
@@ -168,8 +217,10 @@
// Stale lookup.
iref0 = irt.Add(cookie, obj0);
EXPECT_TRUE(iref0 != NULL);
+ CheckDump(&irt, 1, 1);
ASSERT_TRUE(irt.Remove(cookie, iref0));
EXPECT_EQ(kInvalidIndirectRefObject, irt.Get(iref0)) << "stale lookup succeeded";
+ CheckDump(&irt, 0, 0);
// Test table resizing.
// These ones fit...
@@ -177,14 +228,17 @@
for (size_t i = 0; i < kTableInitial; i++) {
manyRefs[i] = irt.Add(cookie, obj0);
ASSERT_TRUE(manyRefs[i] != NULL) << "Failed adding " << i;
+ CheckDump(&irt, i + 1, 1);
}
// ...this one causes overflow.
iref0 = irt.Add(cookie, obj0);
ASSERT_TRUE(iref0 != NULL);
ASSERT_EQ(kTableInitial + 1, irt.Capacity());
+ CheckDump(&irt, kTableInitial + 1, 1);
for (size_t i = 0; i < kTableInitial; i++) {
ASSERT_TRUE(irt.Remove(cookie, manyRefs[i])) << "failed removing " << i;
+ CheckDump(&irt, kTableInitial - i, 1);
}
// Because of removal order, should have 11 entries, 10 of them holes.
ASSERT_EQ(kTableInitial + 1, irt.Capacity());
@@ -192,6 +246,7 @@
ASSERT_TRUE(irt.Remove(cookie, iref0)) << "multi-remove final failed";
ASSERT_EQ(0U, irt.Capacity()) << "multi-del not empty";
+ CheckDump(&irt, 0, 0);
}
} // namespace art
diff --git a/src/reference_table_test.cc b/src/reference_table_test.cc
index 4bb5c97..b35110d 100644
--- a/src/reference_table_test.cc
+++ b/src/reference_table_test.cc
@@ -26,34 +26,79 @@
TEST_F(ReferenceTableTest, Basics) {
ScopedObjectAccess soa(Thread::Current());
Object* o1 = String::AllocFromModifiedUtf8("hello");
- Object* o2 = ShortArray::Alloc(0);
- ReferenceTable rt("test", 0, 4);
- std::ostringstream oss1;
- rt.Dump(oss1);
- EXPECT_TRUE(oss1.str().find("(empty)") != std::string::npos) << oss1.str();
- EXPECT_EQ(0U, rt.Size());
+ ReferenceTable rt("test", 0, 11);
+
+ // Check dumping the empty table.
+ {
+ std::ostringstream oss;
+ rt.Dump(oss);
+ EXPECT_NE(oss.str().find("(empty)"), std::string::npos) << oss.str();
+ EXPECT_EQ(0U, rt.Size());
+ }
+
+ // Check removal of all NULLs in a empty table is a no-op.
rt.Remove(NULL);
EXPECT_EQ(0U, rt.Size());
+
+ // Check removal of all o1 in a empty table is a no-op.
rt.Remove(o1);
EXPECT_EQ(0U, rt.Size());
- rt.Add(o1);
- EXPECT_EQ(1U, rt.Size());
- rt.Add(o2);
- EXPECT_EQ(2U, rt.Size());
- rt.Add(o2);
- EXPECT_EQ(3U, rt.Size());
- std::ostringstream oss2;
- rt.Dump(oss2);
- EXPECT_TRUE(oss2.str().find("Last 3 entries (of 3):") != std::string::npos) << oss2.str();
- EXPECT_TRUE(oss2.str().find("1 of java.lang.String") != std::string::npos) << oss2.str();
- EXPECT_TRUE(oss2.str().find("2 of short[] (1 unique instances)") != std::string::npos) << oss2.str();
- rt.Remove(o1);
- EXPECT_EQ(2U, rt.Size());
- rt.Remove(o2);
- EXPECT_EQ(1U, rt.Size());
- rt.Remove(o2);
- EXPECT_EQ(0U, rt.Size());
+
+ // Add o1 and check we have 1 element and can dump.
+ {
+ rt.Add(o1);
+ EXPECT_EQ(1U, rt.Size());
+ std::ostringstream oss;
+ rt.Dump(oss);
+ EXPECT_NE(oss.str().find("1 of java.lang.String"), std::string::npos) << oss.str();
+ EXPECT_EQ(oss.str().find("short[]"), std::string::npos) << oss.str();
+ }
+
+ // Add a second object 10 times and check dumping is sane.
+ Object* o2 = ShortArray::Alloc(0);
+ for (size_t i = 0; i < 10; ++i) {
+ rt.Add(o2);
+ EXPECT_EQ(i + 2, rt.Size());
+ std::ostringstream oss;
+ rt.Dump(oss);
+ EXPECT_NE(oss.str().find(StringPrintf("Last %zd entries (of %zd):",
+ i + 2 > 10 ? 10 : i + 2,
+ i + 2)),
+ std::string::npos) << oss.str();
+ EXPECT_NE(oss.str().find("1 of java.lang.String"), std::string::npos) << oss.str();
+ if (i == 0) {
+ EXPECT_NE(oss.str().find("1 of short[]"), std::string::npos) << oss.str();
+ } else {
+ EXPECT_NE(oss.str().find(StringPrintf("%zd of short[] (1 unique instances)", i + 1)),
+ std::string::npos) << oss.str();
+ }
+ }
+
+ // Remove o1 (first element).
+ {
+ rt.Remove(o1);
+ EXPECT_EQ(10U, rt.Size());
+ std::ostringstream oss;
+ rt.Dump(oss);
+ EXPECT_EQ(oss.str().find("java.lang.String"), std::string::npos) << oss.str();
+ }
+
+ // Remove o2 ten times.
+ for (size_t i = 0; i < 10; ++i) {
+ rt.Remove(o2);
+ EXPECT_EQ(9 - i, rt.Size());
+ std::ostringstream oss;
+ rt.Dump(oss);
+ if (i == 9) {
+ EXPECT_EQ(oss.str().find("short[]"), std::string::npos) << oss.str();
+ } else if (i == 8) {
+ EXPECT_NE(oss.str().find("1 of short[]"), std::string::npos) << oss.str();
+ } else {
+ EXPECT_NE(oss.str().find(StringPrintf("%zd of short[] (1 unique instances)", 10 - i - 1)),
+ std::string::npos) << oss.str();
+ }
+ }
}
} // namespace art
