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

3 files changed, 131 insertions, 25 deletions

diff --git a/src/indirect_reference_table.cc b/src/indirect_reference_table.cc
index 958531df56..9bb6edc9cb 100644
--- a/src/indirect_reference_table.cc
+++ b/src/indirect_reference_table.cc
@@ -318,6 +318,12 @@ void IndirectReferenceTable::VisitRoots(Heap::RootVisitor* visitor, void* arg) {
 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 1698f18612..971e42a3dc 100644
--- a/src/indirect_reference_table_test.cc
+++ b/src/indirect_reference_table_test.cc
@@ -23,6 +23,24 @@ namespace art {
 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 @@ TEST_F(IndirectReferenceTableTest, BasicTest) {
 
   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 @@ TEST_F(IndirectReferenceTableTest, BasicTest) {
   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 @@ TEST_F(IndirectReferenceTableTest, BasicTest) {
   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 @@ TEST_F(IndirectReferenceTableTest, BasicTest) {
   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 @@ TEST_F(IndirectReferenceTableTest, BasicTest) {
   // 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 @@ TEST_F(IndirectReferenceTableTest, BasicTest) {
   // 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 @@ TEST_F(IndirectReferenceTableTest, BasicTest) {
   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 @@ TEST_F(IndirectReferenceTableTest, BasicTest) {
   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 4bb5c97ec4..b35110d21d 100644
--- a/src/reference_table_test.cc
+++ b/src/reference_table_test.cc
@@ -26,34 +26,79 @@ class ReferenceTableTest : public CommonTest {
 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