android: crazy-linker: Minor cleanups and optimization.

Nothin too sexy here, but making small improvements to the
code base for long term maintainer happiness, and upcoming
CLs that will perform more deep cleanup work.

- Adding missing const qualifiers to some LibraryView
  methods. Note that making LookupSymbol() also const
  requires more changes that are better left to another
  CL. Noted with TODO.

- Make crazy::SearchResult declaration public by moving
  it to crazy_linker_util.h

- Use a VectorBase non-template class that all Vector<>
  templates inherit from, in order to reduce the amount
  of generated code for vectors.

- Replace Vector::IndexOf() with Vector::Find() which
  returns a SearchResult value. Gets rid of pesky
  'int' indices for InsertAt / RemoveAt method.

- Support ranged-based for loops with begin() / end()
  declarations. And adjust all callers to use them
  when that makes sense. Cleaner code. Yeah.

BUG=NONE
R=lizeb@chromium.org, agrieve@chromium.org, pasko@chromium.org, rmcilroy@chromium.org

android: crazy-linker: Small Vector<> optimization.
Change-Id: If4d22b85fdd44885159a2bb27626f2942ccb437e
Reviewed-on: https://chromium-review.googlesource.com/1090840
Commit-Queue: David Turner <digit@chromium.org>
Reviewed-by: Egor Pasko <pasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565329}

third_party/android_crazy_linker/src/src/crazy_linker_library_list.cpp

@@ -95,8 +95,8 @@ LibraryList::~LibraryList() {
 
   // Destroy all known libraries.
   while (!known_libraries_.IsEmpty()) {
-    LibraryView* wrap = known_libraries_.PopLast();
-    delete wrap;
+    LibraryView* view = known_libraries_.PopLast();
+    delete view;
   }
 }
 
@@ -149,9 +149,8 @@ void LibraryList::LoadPreloads() {
 
   if (CRAZY_DEBUG) {
     LOG("Preloads loaded");
-    for (size_t n = 0; n < preloaded_libraries_.GetCount(); ++n)
-      LOG("  ... %p %s\n",
-          preloaded_libraries_[n], preloaded_libraries_[n]->GetName());
+    for (const LibraryView* preload : preloaded_libraries_)
+      LOG("  ... %p %s\n", preload, preload->GetName());
     LOG("    preloads @%p\n", &preloaded_libraries_);
   }
 }
@@ -161,12 +160,11 @@ LibraryView* LibraryList::FindLibraryByName(const char* lib_name) {
   if (!lib_name)
     return NULL;
 
-  for (size_t n = 0; n < known_libraries_.GetCount(); ++n) {
-    LibraryView* wrap = known_libraries_[n];
-    if (!strcmp(lib_name, wrap->GetName()))
-      return wrap;
+  for (LibraryView* view : known_libraries_) {
+    if (!strcmp(lib_name, view->GetName()))
+      return view;
   }
-  return NULL;
+  return nullptr;
 }
 
 void* LibraryList::FindSymbolFrom(const char* symbol_name, LibraryView* from) {
@@ -224,13 +222,12 @@ LibraryView* LibraryList::FindLibraryForAddress(void* address) {
   // Linearly scan all libraries, looking for one that contains
   // a given address. NOTE: This doesn't check that this falls
   // inside one of the mapped library segments.
-  for (size_t n = 0; n < known_libraries_.GetCount(); ++n) {
-    LibraryView* wrap = known_libraries_[n];
+  for (LibraryView* view : known_libraries_) {
     // TODO(digit): Search addresses inside system libraries.
-    if (wrap->IsCrazy()) {
-      SharedLibrary* lib = wrap->GetCrazy();
+    if (view->IsCrazy()) {
+      SharedLibrary* lib = view->GetCrazy();
       if (lib->ContainsAddress(address))
-        return wrap;
+        return view;
     }
   }
   return NULL;
@@ -402,8 +399,8 @@ LibraryView* LibraryList::LoadLibrary(const char* lib_name,
   }
   if (CRAZY_DEBUG) {
     LOG("Dependencies loaded for %s", base_name);
-    for (size_t n = 0; n < dependencies.GetCount(); ++n)
-      LOG("  ... %p %s\n", dependencies[n], dependencies[n]->GetName());
+    for (const LibraryView* dep : dependencies)
+      LOG("  ... %p %s\n", dep, dep->GetName());
     LOG("    dependencies @%p\n", &dependencies);
   }
 
@@ -520,10 +517,9 @@ void LibraryList::AddLibrary(LibraryView* wrap) {
 
 LibraryView* LibraryList::FindKnownLibrary(const char* name) {
   const char* base_name = GetBaseNamePtr(name);
-  for (size_t n = 0; n < known_libraries_.GetCount(); ++n) {
-    LibraryView* wrap = known_libraries_[n];
-    if (!strcmp(base_name, wrap->GetName()))
-      return wrap;
+  for (LibraryView* view : known_libraries_) {
+    if (!strcmp(base_name, view->GetName()))
+      return view;
   }
   return NULL;
 }

third_party/android_crazy_linker/src/src/crazy_linker_library_view.cpp

@@ -44,7 +44,7 @@ bool LibraryView::GetInfo(size_t* load_address,
                           size_t* load_size,
                           size_t* relro_start,
                           size_t* relro_size,
-                          Error* error) {
+                          Error* error) const {
   if (type_ != TYPE_CRAZY) {
     *error = "No RELRO sharing with system libraries";
     return false;

third_party/android_crazy_linker/src/src/crazy_linker_library_view.h

@@ -53,11 +53,11 @@ class LibraryView {
 
   // Returns the soname of the current library (or its base name if not
   // available).
-  const char* GetName() { return name_.c_str(); }
+  const char* GetName() const { return name_.c_str(); }
 
-  SharedLibrary* GetCrazy() { return IsCrazy() ? crazy_ : NULL; }
+  SharedLibrary* GetCrazy() const { return IsCrazy() ? crazy_ : NULL; }
 
-  void* GetSystem() { return IsSystem() ? system_ : NULL; }
+  void* GetSystem() const { return IsSystem() ? system_ : NULL; }
 
   // Increment reference count for this LibraryView.
   void AddRef() { ref_count_++; }
@@ -69,6 +69,7 @@ class LibraryView {
   // Lookup a symbol from this library.
   // If this is a crazy library, perform a breadth-first search,
   // for system libraries, use dlsym() instead.
+  // TODO(digit): Make this const.
   void* LookupSymbol(const char* symbol_name);
 
   // Retrieve library information.
@@ -76,7 +77,7 @@ class LibraryView {
                size_t* load_size,
                size_t* relro_start,
                size_t* relro_size,
-               Error* error);
+               Error* error) const;
 
   // Only used for debugging.
   int ref_count() const { return ref_count_; }

third_party/android_crazy_linker/src/src/crazy_linker_pointer_set.cpp

@@ -6,13 +6,6 @@
 
 namespace crazy {
 
-// Result type for the BinarySearch function below.
-// The packing generates smaller and faster machine code on ARM and x86.
-struct SearchResult {
-  bool found : 1;
-  size_t pos : sizeof(size_t) * CHAR_BIT - 1;
-};
-
 static SearchResult BinarySearch(const Vector<void*>& items, void* key) {
   auto key_val = reinterpret_cast<uintptr_t>(key);
   size_t min = 0, max = items.GetCount();

third_party/android_crazy_linker/src/src/crazy_linker_proc_maps.cpp

@@ -174,8 +174,7 @@ class ProcMapsInternal {
 
  private:
   void Reset() {
-    for (size_t n = 0; n < entries_.GetCount(); ++n) {
-      ProcMaps::Entry& entry = entries_[n];
+    for (ProcMaps::Entry& entry : entries_) {
       ::free(const_cast<char*>(entry.path));
     }
     entries_.Resize(0);

third_party/android_crazy_linker/src/src/crazy_linker_util.cpp

@@ -115,4 +115,86 @@ void String::InitFrom(const char* str, size_t len) {
   }
 }
 
+VectorBase::~VectorBase() {
+  ::free(data_);
+}
+
+VectorBase::VectorBase(VectorBase&& other)
+    : data_(other.data_), count_(other.count_), capacity_(other.capacity_) {
+  other.DoReset();
+}
+
+VectorBase& VectorBase::operator=(VectorBase&& other) {
+  if (&other != this) {
+    ::free(data_);
+    data_ = other.data_;
+    count_ = other.count_;
+    capacity_ = other.capacity_;
+    other.DoReset();
+  }
+  return *this;
+}
+
+void VectorBase::DoResize(size_t new_count, size_t item_size) {
+  if (new_count > capacity_) {
+    DoReserve(new_count, item_size);
+  }
+  if (new_count > count_)
+    ::memset(data_ + count_ * item_size, 0, (new_count - count_) * item_size);
+
+  count_ = new_count;
+}
+
+void VectorBase::DoReserve(size_t new_capacity, size_t item_size) {
+  data_ = static_cast<char*>(::realloc(data_, new_capacity * item_size));
+  capacity_ = new_capacity;
+  if (count_ > capacity_)
+    count_ = capacity_;
+}
+
+void* VectorBase::DoInsert(size_t pos, size_t item_size) {
+  return DoInsert(pos, 1, item_size);
+}
+
+void* VectorBase::DoInsert(size_t pos, size_t count, size_t item_size) {
+  if (pos >= count_)
+    pos = count_;
+
+  size_t new_count = count_ + count;
+  if (new_count > capacity_) {
+    size_t new_capacity = capacity_;
+    while (new_capacity < new_count) {
+      new_capacity += (new_capacity >> 2) + 4;
+    }
+    DoReserve(new_capacity, item_size);
+  }
+
+  char* from_data = data_ + pos * item_size;
+  char* to_data = from_data + count * item_size;
+
+  ::memmove(to_data, from_data, (count_ - pos) * item_size);
+  ::memset(from_data, 0, count * item_size);
+
+  count_ = new_count;
+  return from_data;
+}
+
+void VectorBase::DoRemove(size_t pos, size_t item_size) {
+  DoRemove(pos, 1, item_size);
+}
+
+void VectorBase::DoRemove(size_t pos, size_t count, size_t item_size) {
+  if (pos >= count_)
+    return;
+
+  if (count > count_ - pos)
+    count = count_ - pos;
+
+  char* to_data = data_ + pos * item_size;
+  char* from_data = to_data + count * item_size;
+
+  ::memmove(to_data, from_data, (count_ - pos - count) * item_size);
+  count_ -= count;
+}
+
 }  // namespace crazy

third_party/android_crazy_linker/src/src/crazy_linker_util.h

@@ -6,12 +6,16 @@
 #define CRAZY_LINKER_UTIL_H
 
 #include <fcntl.h>
+#include <limits.h>
 #include <stdarg.h>
 #include <stdio.h>
 #include <unistd.h>
 #include <cstdlib>
 #include <cstring>
 
+#include <type_traits>
+#include <utility>
+
 namespace crazy {
 
 // Helper macro to loop around EINTR errors in syscalls.
@@ -161,6 +165,55 @@ class String {
   size_t capacity_;
 };
 
+// Base vector class used by all instantiations of Vector<> to reduce
+// generated code size.
+class VectorBase {
+ public:
+  VectorBase() = default;
+  ~VectorBase();
+
+  // Disallow copy operations.
+  VectorBase(const VectorBase&) = delete;
+  VectorBase& operator=(const VectorBase&) = delete;
+
+  // Allow move operations.
+  VectorBase(VectorBase&& other);
+  VectorBase& operator=(VectorBase&& other);
+
+  // Return true iff vector is empty.
+  constexpr bool IsEmpty() const { return count_ == 0U; }
+
+  // Return number of items in container.
+  constexpr size_t GetCount() const { return count_; }
+
+ protected:
+  // Reset container state.
+  void DoReset() {
+    data_ = nullptr;
+    count_ = 0;
+    capacity_ = 0;
+  }
+
+  // Perform various operations on the array.
+  void DoResize(size_t new_count, size_t item_size);
+  void DoReserve(size_t new_capacity, size_t item_size);
+  void* DoInsert(size_t pos, size_t item_size);
+  void* DoInsert(size_t pos, size_t count, size_t item_size);
+  void DoRemove(size_t pos, size_t item_size);
+  void DoRemove(size_t pos, size_t count, size_t item_size);
+
+  char* data_ = nullptr;
+  size_t count_ = 0;
+  size_t capacity_ = 0;
+};
+
+// Result type for a linear or binary search function.
+// The packing generates smaller and faster machine code on ARM and x86.
+struct SearchResult {
+  bool found : 1;
+  size_t pos : sizeof(size_t) * CHAR_BIT - 1;
+};
+
 // Helper template used to implement a simple vector of POD-struct items.
 // I.e. this uses memmove() to move items during insertion / removal.
 //
@@ -168,152 +221,91 @@ class String {
 // libstdc++ which only provides new/delete.
 //
 template <class T>
-class Vector {
+class Vector : public VectorBase {
  public:
-  Vector() = default;
+  Vector() { static_assert(std::is_pod<T>::value, "type T should be POD"); }
 
-  ~Vector() { free(items_); }
+  ~Vector() = default;
 
-  // For now, prevent copy operations. They may be implemented later if
-  // needed.
-  Vector(const Vector& other) = delete;
-  Vector& operator=(const Vector& other) = delete;
+  // Move operations are allowed.
+  Vector(Vector&& other) : VectorBase(std::move(other)) {}
 
-  // Allow move operations.
-  Vector(Vector&& other);
-  Vector& operator=(Vector&& other);
+  Vector& operator=(Vector&& other) {
+    if (this != &other) {
+      this->VectorBase::operator=(std::move(other));
+    }
+    return *this;
+  }
+
+  // Support for-range loops.
+  constexpr const T* cbegin() const {
+    return reinterpret_cast<const T*>(data_);
+  }
+  constexpr const T* cend() const { return cbegin() + count_; }
 
-  const T& operator[](size_t index) const { return items_[index]; }
+  constexpr const T* begin() const { return cbegin(); }
+  constexpr const T* end() const { return cend(); }
 
-  T& operator[](size_t index) { return items_[index]; }
+  T* begin() { return const_cast<T*>(cbegin()); }
+  T* end() { return const_cast<T*>(cend()); }
 
-  bool IsEmpty() const { return count_ == 0; }
+  // Array access operator.
+  constexpr const T& operator[](size_t index) const { return cbegin()[index]; }
+  T& operator[](size_t index) { return begin()[index]; }
 
-  void PushBack(T item) { InsertAt(static_cast<int>(count_), item); }
+  // Append one item at the end of the vector.
+  void PushBack(T item) { InsertAt(count_, item); }
 
+  // Remove the first item from the vector and return it.
+  // Undefined behaviour if it is empty.
   T PopFirst() {
-    T result = items_[0];
+    T result = cbegin()[0];
     RemoveAt(0);
     return result;
   }
 
+  // Remove the last item from the vector and return it.
+  // Undefined behaviour if the vector is empty.
   T PopLast() {
-    T result = items_[count_ - 1];
-    Resize(count_ - 1);
+    T result = cend()[-1];
+    DoResize(count_ - 1, sizeof(T));
     return result;
   }
 
+  // Remove a specific item from the vector, if it contains it.
   void Remove(T item) {
-    int index = IndexOf(item);
-    if (index >= 0)
-      RemoveAt(index);
-  }
-
-  void InsertAt(int index, T item);
-
-  void RemoveAt(int index);
-
-  int IndexOf(T item) const;
-
-  bool Has(T item) const { return IndexOf(item) >= 0; }
-
-  size_t GetCount() const { return count_; }
-
-  void Reserve(size_t new_capacity);
-
-  void Resize(size_t new_count);
-
- private:
-  void DoReset() {
-    items_ = nullptr;
-    count_ = 0;
-    capacity_ = 0;
+    SearchResult result = Find(item);
+    if (result.found)
+      RemoveAt(result.pos);
   }
 
-  T* items_ = nullptr;
-  size_t count_ = 0;
-  size_t capacity_ = 0;
-};
-
-template <class T>
-int Vector<T>::IndexOf(T item) const {
-  for (size_t n = 0; n < count_; ++n) {
-    if (items_[n] == item)
-      return static_cast<int>(n);
+  // Insert a new |item| into a specific position |index|.
+  void InsertAt(size_t index, T item) {
+    auto* slot = reinterpret_cast<T*>(DoInsert(index, sizeof(T)));
+    *slot = item;
   }
-  return -1;
-}
 
-template <class T>
-Vector<T>::Vector(Vector&& other)
-    : items_(other.items_), count_(other.count_), capacity_(other.capacity_) {
-  other.items_ = nullptr;
-  other.count_ = 0;
-  other.capacity_ = 0;
-}
+  // Remove the item at position |index|.
+  void RemoveAt(size_t index) { DoRemove(index, sizeof(T)); }
 
-template <class T>
-Vector<T>& Vector<T>::operator=(Vector&& other) {
-  if (this != &other) {
-    free(items_);
-    items_ = other.items_;
-    count_ = other.count_;
-    capacity_ = other.capacity_;
-    other.items_ = nullptr;
-    other.count_ = 0;
-    other.capacity_ = 0;
+  // Try to find |item| in the vector.
+  SearchResult Find(T wanted) const {
+    for (size_t pos = 0; pos < count_; ++pos) {
+      if (cbegin()[pos] == wanted)
+        return {true, pos};
+    }
+    return {false, 0};
   }
-  return *this;
-}
-
-template <class T>
-void Vector<T>::InsertAt(int index, T item) {
-  if (count_ >= capacity_)
-    Reserve(capacity_ + (capacity_ >> 1) + 4);
-
-  if (index < 0)
-    index = 0;
-  size_t n = static_cast<size_t>(index);
-  if (n > count_)
-    n = count_;
-  else
-    memmove(items_ + n + 1, items_ + n, (count_ - n) * sizeof(T));
-
-  items_[n] = item;
-  count_++;
-}
-
-template <class T>
-void Vector<T>::RemoveAt(int index) {
-  if (index < 0)
-    return;
-
-  size_t n = static_cast<size_t>(index);
-  if (n >= count_)
-    return;
 
-  memmove(items_ + n, items_ + n + 1, (count_ - n - 1) * sizeof(T));
-  count_--;
-}
+  // Returns true if vector contains |item|.
+  bool Has(T item) const { return Find(item).found; }
 
-template <class T>
-void Vector<T>::Reserve(size_t new_capacity) {
-  items_ = reinterpret_cast<T*>(realloc(items_, new_capacity * sizeof(T)));
-  capacity_ = new_capacity;
-  if (count_ > capacity_)
-    count_ = capacity_;
-}
+  // Resize vector.
+  void Resize(size_t new_count) { DoResize(new_count, sizeof(T)); }
 
-template <class T>
-void Vector<T>::Resize(size_t new_size) {
-  if (new_size > capacity_)
-    Reserve(new_size);
-
-  if (new_size > count_)
-    memset(items_ + count_, 0, (new_size - count_) * sizeof(T));
-
-  count_ = new_size;
-}
+  // Reset the vector's capacity to a new value. Truncate size if needed.
+  void Reserve(size_t new_capacity) { DoReserve(new_capacity, sizeof(T)); }
+};
 
 }  // namespace crazy
 

third_party/android_crazy_linker/src/src/crazy_linker_util_unittest.cpp

@@ -185,14 +185,29 @@ TEST(Vector, At) {
   }
 }
 
-TEST(Vector, IndexOf) {
+TEST(Vector, Find) {
   const int kMaxCount = 500;
   Vector<int> v;
   for (int n = 0; n < kMaxCount; ++n)
     v.PushBack(n * 100);
 
   for (int n = 0; n < kMaxCount; ++n) {
-    EXPECT_EQ(n, v.IndexOf(n * 100)) << "Checking v.IndexOf(" << n * 100 << ")";
+    SearchResult r = v.Find(n * 100);
+    EXPECT_TRUE(r.found) << "Looking for " << n * 100;
+    EXPECT_EQ(n, r.pos) << "Looking for " << n * 100;
+  }
+}
+
+TEST(Vector, ForRangeLoop) {
+  const int kMaxCount = 500;
+  Vector<int> v;
+  for (int n = 0; n < kMaxCount; ++n)
+    v.PushBack(n * 100);
+
+  int n = 0;
+  for (const int& value : v) {
+    EXPECT_EQ(n * 100, value) << "Checking v[" << n << "]";
+    n++;
   }
 }