Erase elements in LinkedList::remove_if

Change-Id: I5119a78c73ffe780a81c53ab5ff0266d5c82d319

linker/linked_list.h

@@ -88,24 +88,50 @@ class LinkedList {
   template<typename F>
   void for_each(F&& action) {
     for (LinkedListEntry<T>* e = head_; e != nullptr; e = e->next) {
-      if (e->element != nullptr) {
       action(e->element);
     }
   }
-  }
 
   template<typename F>
-  void remove_if(F&& predicate) {
+  void remove_if(F predicate) {
-    for (LinkedListEntry<T>* e = head_; e != nullptr; e = e->next) {
+    for (LinkedListEntry<T>* e = head_, *p = nullptr; e != nullptr;) {
-      if (e->element != nullptr && predicate(e->element)) {
+      if (predicate(e->element)) {
-        e->element = nullptr;
+        LinkedListEntry<T>* next = e->next;
+        if (p == nullptr) {
+          head_ = next;
+        } else {
+          p->next = next;
+        }
+        Allocator::free(e);
+        e = next;
+      } else {
+        p = e;
+        e = e->next;
       }
     }
   }
 
-  bool contains(const T* el) {
+  size_t size() const {
+    size_t sz = 0;
     for (LinkedListEntry<T>* e = head_; e != nullptr; e = e->next) {
-      if (e->element != nullptr && e->element == el) {
+      ++sz;
+    }
+
+    return sz;
+  }
+
+  size_t copy_to_array(T* array[], size_t array_length) const {
+    size_t sz = 0;
+    for (LinkedListEntry<T>* e = head_; sz < array_length && e != nullptr; e = e->next) {
+      array[sz++] = e->element;
+    }
+
+    return sz;
+  }
+
+  bool contains(const T* el) const {
+    for (LinkedListEntry<T>* e = head_; e != nullptr; e = e->next) {
+      if (e->element == el) {
         return true;
       }
     }

linker/linker.cpp

@@ -609,6 +609,8 @@ done:
     return NULL;
 }
 
+
+
 // Another soinfo list allocator to use in dlsym. We don't reuse
 // SoinfoListAllocator because it is write-protected most of the time.
 static LinkerAllocator<LinkedListEntry<soinfo>> g_soinfo_list_allocator_rw;
@@ -861,10 +863,17 @@ static void soinfo_unload(soinfo* si) {
     si->CallDestructors();
 
     if (si->has_min_version(0)) {
-      si->get_children().for_each([&] (soinfo* child) {
+      // It is not safe to do si->get_children().for_each, because
-        TRACE("%s needs to unload %s", si->name, child->name);
+      // during soinfo_free the child will concurrently modify the si->children
-        soinfo_unload(child);
+      // list, therefore we create a copy and use it to unload children.
-      });
+      size_t children_count = si->get_children().size();
+      soinfo* children[children_count];
+      si->get_children().copy_to_array(children, children_count);
+
+      for (size_t i = 0; i < children_count; ++i) {
+        TRACE("%s needs to unload %s", si->name, children[i]->name);
+        soinfo_unload(children[i]);
+      }
     } else {
       for (ElfW(Dyn)* d = si->dynamic; d->d_tag != DT_NULL; ++d) {
         if (d->d_tag == DT_NEEDED) {
@@ -1618,7 +1627,7 @@ void soinfo::remove_all_links() {
   });
 
   parents.for_each([&] (soinfo* parent) {
-    parent->children.for_each([&] (const soinfo* child) {
+    parent->children.remove_if([&] (const soinfo* child) {
       return child == this;
     });
   });

linker/tests/linked_list_test.cpp

@@ -80,7 +80,7 @@ TEST(linked_list, simple) {
   });
 
   ASSERT_TRUE(!alloc_called);
-  ASSERT_TRUE(!free_called);
+  ASSERT_TRUE(free_called);
 
   ASSERT_EQ("dba", test_list_to_string(list));
   alloc_called = free_called = false;
@@ -103,15 +103,82 @@ TEST(linked_list, push_pop) {
   ASSERT_EQ("ab", test_list_to_string(list));
   list.push_back("c");
   ASSERT_EQ("abc", test_list_to_string(list));
-  ASSERT_EQ("a", list.pop_front());
+  ASSERT_STREQ("a", list.pop_front());
   ASSERT_EQ("bc", test_list_to_string(list));
-  ASSERT_EQ("b", list.pop_front());
+  ASSERT_STREQ("b", list.pop_front());
   ASSERT_EQ("c", test_list_to_string(list));
-  ASSERT_EQ("c", list.pop_front());
+  ASSERT_STREQ("c", list.pop_front());
   ASSERT_EQ("", test_list_to_string(list));
   ASSERT_TRUE(list.pop_front() == nullptr);
   list.push_back("r");
   ASSERT_EQ("r", test_list_to_string(list));
-  ASSERT_EQ("r", list.pop_front());
+  ASSERT_STREQ("r", list.pop_front());
   ASSERT_TRUE(list.pop_front() == nullptr);
 }
+
+TEST(linked_list, remove_if_then_pop) {
+  test_list_t list;
+  list.push_back("a");
+  list.push_back("b");
+  list.push_back("c");
+  list.push_back("d");
+  list.remove_if([](const char* c) {
+    return *c == 'b' || *c == 'c';
+  });
+
+  ASSERT_EQ("ad", test_list_to_string(list));
+  ASSERT_STREQ("a", list.pop_front());
+  ASSERT_EQ("d", test_list_to_string(list));
+  ASSERT_STREQ("d", list.pop_front());
+  ASSERT_TRUE(list.pop_front() == nullptr);
+}
+
+TEST(linked_list, copy_to_array) {
+  test_list_t list;
+  const size_t max_size = 128;
+  const char* buf[max_size];
+  memset(buf, 0, sizeof(buf));
+
+  ASSERT_EQ(0U, list.size());
+  ASSERT_EQ(0U, list.copy_to_array(buf, max_size));
+  ASSERT_EQ(nullptr, buf[0]);
+
+  list.push_back("a");
+  list.push_back("b");
+  list.push_back("c");
+  list.push_back("d");
+
+  memset(buf, 0, sizeof(buf));
+  ASSERT_EQ(4U, list.size());
+  ASSERT_EQ(2U, list.copy_to_array(buf, 2));
+  ASSERT_EQ('a', *buf[0]);
+  ASSERT_EQ('b', *buf[1]);
+  ASSERT_EQ(nullptr, buf[2]);
+
+  ASSERT_EQ(4U, list.copy_to_array(buf, max_size));
+  ASSERT_EQ('a', *buf[0]);
+  ASSERT_EQ('b', *buf[1]);
+  ASSERT_EQ('c', *buf[2]);
+  ASSERT_EQ('d', *buf[3]);
+  ASSERT_EQ(nullptr, buf[4]);
+
+  memset(buf, 0, sizeof(buf));
+  list.remove_if([](const char* c) {
+    return *c != 'c';
+  });
+  ASSERT_EQ(1U, list.size());
+  ASSERT_EQ(1U, list.copy_to_array(buf, max_size));
+  ASSERT_EQ('c', *buf[0]);
+  ASSERT_EQ(nullptr, buf[1]);
+
+  memset(buf, 0, sizeof(buf));
+
+  list.remove_if([](const char* c) {
+    return *c == 'c';
+  });
+
+  ASSERT_EQ(0U, list.size());
+  ASSERT_EQ(0U, list.copy_to_array(buf, max_size));
+  ASSERT_EQ(nullptr, buf[0]);
+}
+