Merge "Modify elf cache to handle elf_offsets properly."

am: 277a95bfef

Change-Id: I71c15df8d527548610cc15484f7c382e85b54552

libunwindstack/Elf.cpp

@@ -20,6 +20,7 @@
 #include <memory>
 #include <mutex>
 #include <string>
+#include <utility>
 
 #define LOG_TAG "unwind"
 #include <log/log.h>
@@ -36,7 +37,7 @@
 namespace unwindstack {
 
 bool Elf::cache_enabled_;
-std::unordered_map<std::string, std::shared_ptr<Elf>>* Elf::cache_;
+std::unordered_map<std::string, std::pair<std::shared_ptr<Elf>, bool>>* Elf::cache_;
 std::mutex* Elf::cache_lock_;
 
 bool Elf::Init(bool init_gnu_debugdata) {
@@ -308,7 +309,7 @@ uint64_t Elf::GetLoadBias(Memory* memory) {
 void Elf::SetCachingEnabled(bool enable) {
   if (!cache_enabled_ && enable) {
     cache_enabled_ = true;
-    cache_ = new std::unordered_map<std::string, std::shared_ptr<Elf>>;
+    cache_ = new std::unordered_map<std::string, std::pair<std::shared_ptr<Elf>, bool>>;
     cache_lock_ = new std::mutex;
   } else if (cache_enabled_ && !enable) {
     cache_enabled_ = false;
@@ -326,18 +327,54 @@ void Elf::CacheUnlock() {
 }
 
 void Elf::CacheAdd(MapInfo* info) {
-  if (info->offset == 0) {
-    (*cache_)[info->name] = info->elf;
-  } else {
-    std::string name(info->name + ':' + std::to_string(info->offset));
-    (*cache_)[name] = info->elf;
+  // If elf_offset != 0, then cache both name:offset and name.
+  // The cached name is used to do lookups if multiple maps for the same
+  // named elf file exist.
+  // For example, if there are two maps boot.odex:1000 and boot.odex:2000
+  // where each reference the entire boot.odex, the cache will properly
+  // use the same cached elf object.
+
+  if (info->offset == 0 || info->elf_offset != 0) {
+    (*cache_)[info->name] = std::make_pair(info->elf, true);
+  }
+
+  if (info->offset != 0) {
+    // The second element in the pair indicates whether elf_offset should
+    // be set to offset when getting out of the cache.
+    (*cache_)[info->name + ':' + std::to_string(info->offset)] =
+        std::make_pair(info->elf, info->elf_offset != 0);
   }
 }
 
-bool Elf::CacheGet(const std::string& name, std::shared_ptr<Elf>* elf) {
+bool Elf::CacheAfterCreateMemory(MapInfo* info) {
+  if (info->name.empty() || info->offset == 0 || info->elf_offset == 0) {
+    return false;
+  }
+
+  auto entry = cache_->find(info->name);
+  if (entry == cache_->end()) {
+    return false;
+  }
+
+  // In this case, the whole file is the elf, and the name has already
+  // been cached. Add an entry at name:offset to get this directly out
+  // of the cache next time.
+  info->elf = entry->second.first;
+  (*cache_)[info->name + ':' + std::to_string(info->offset)] = std::make_pair(info->elf, true);
+  return true;
+}
+
+bool Elf::CacheGet(MapInfo* info) {
+  std::string name(info->name);
+  if (info->offset != 0) {
+    name += ':' + std::to_string(info->offset);
+  }
   auto entry = cache_->find(name);
   if (entry != cache_->end()) {
-    *elf = entry->second;
+    info->elf = entry->second.first;
+    if (entry->second.second) {
+      info->elf_offset = info->offset;
+    }
     return true;
   }
   return false;

libunwindstack/MapInfo.cpp

@@ -117,23 +117,15 @@ Elf* MapInfo::GetElf(const std::shared_ptr<Memory>& process_memory, bool init_gn
   if (Elf::CachingEnabled() && !name.empty()) {
     Elf::CacheLock();
     locked = true;
-    if (offset != 0) {
-      std::string hash(name + ':' + std::to_string(offset));
-      if (Elf::CacheGet(hash, &elf)) {
-        Elf::CacheUnlock();
-        return elf.get();
-      }
-    } else if (Elf::CacheGet(name, &elf)) {
+    if (Elf::CacheGet(this)) {
       Elf::CacheUnlock();
       return elf.get();
     }
   }
 
   Memory* memory = CreateMemory(process_memory);
-  if (locked && offset != 0 && elf_offset != 0) {
-    // In this case, the whole file is the elf, need to see if the elf
-    // data was cached.
-    if (Elf::CacheGet(name, &elf)) {
+  if (locked) {
+    if (Elf::CacheAfterCreateMemory(this)) {
       delete memory;
       Elf::CacheUnlock();
       return elf.get();

libunwindstack/include/unwindstack/Elf.h

@@ -23,6 +23,7 @@
 #include <mutex>
 #include <string>
 #include <unordered_map>
+#include <utility>
 
 #include <unwindstack/ElfInterface.h>
 #include <unwindstack/Memory.h>
@@ -103,7 +104,8 @@ class Elf {
   static void CacheLock();
   static void CacheUnlock();
   static void CacheAdd(MapInfo* info);
-  static bool CacheGet(const std::string& name, std::shared_ptr<Elf>* elf);
+  static bool CacheGet(MapInfo* info);
+  static bool CacheAfterCreateMemory(MapInfo* info);
 
  protected:
   bool valid_ = false;
@@ -120,7 +122,7 @@ class Elf {
   std::unique_ptr<ElfInterface> gnu_debugdata_interface_;
 
   static bool cache_enabled_;
-  static std::unordered_map<std::string, std::shared_ptr<Elf>>* cache_;
+  static std::unordered_map<std::string, std::pair<std::shared_ptr<Elf>, bool>>* cache_;
   static std::mutex* cache_lock_;
 };
 

libunwindstack/tests/ElfCacheTest.cpp

@@ -60,6 +60,7 @@ class ElfCacheTest : public ::testing::Test {
 
   void VerifyWithinSameMap(bool cache_enabled);
   void VerifySameMap(bool cache_enabled);
+  void VerifyWithinSameMapNeverReadAtZero(bool cache_enabled);
 
   static std::shared_ptr<Memory> memory_;
 };
@@ -198,4 +199,66 @@ TEST_F(ElfCacheTest, caching_valid_elf_offset_non_zero) {
   VerifyWithinSameMap(true);
 }
 
+// Verify that when reading from multiple non-zero offsets in the same map
+// that when cached, all of the elf objects are the same.
+void ElfCacheTest::VerifyWithinSameMapNeverReadAtZero(bool cache_enabled) {
+  if (!cache_enabled) {
+    Elf::SetCachingEnabled(false);
+  }
+
+  TemporaryFile tf;
+  ASSERT_TRUE(tf.fd != -1);
+  WriteElfFile(0, &tf, EM_ARM);
+  lseek(tf.fd, 0x500, SEEK_SET);
+  uint8_t value = 0;
+  write(tf.fd, &value, 1);
+  close(tf.fd);
+
+  uint64_t start = 0x1000;
+  uint64_t end = 0x20000;
+  // Multiple info sections at different offsets will have non-zero elf offsets.
+  MapInfo info300_1(start, end, 0x300, 0x5, tf.path);
+  MapInfo info300_2(start, end, 0x300, 0x5, tf.path);
+  MapInfo info400_1(start, end, 0x400, 0x5, tf.path);
+  MapInfo info400_2(start, end, 0x400, 0x5, tf.path);
+
+  Elf* elf300_1 = info300_1.GetElf(memory_, true);
+  ASSERT_TRUE(elf300_1->valid());
+  EXPECT_EQ(ARCH_ARM, elf300_1->arch());
+  Elf* elf300_2 = info300_2.GetElf(memory_, true);
+  ASSERT_TRUE(elf300_2->valid());
+  EXPECT_EQ(ARCH_ARM, elf300_2->arch());
+  EXPECT_EQ(0x300U, info300_1.elf_offset);
+  EXPECT_EQ(0x300U, info300_2.elf_offset);
+  if (cache_enabled) {
+    EXPECT_EQ(elf300_1, elf300_2);
+  } else {
+    EXPECT_NE(elf300_1, elf300_2);
+  }
+
+  Elf* elf400_1 = info400_1.GetElf(memory_, true);
+  ASSERT_TRUE(elf400_1->valid());
+  EXPECT_EQ(ARCH_ARM, elf400_1->arch());
+  Elf* elf400_2 = info400_2.GetElf(memory_, true);
+  ASSERT_TRUE(elf400_2->valid());
+  EXPECT_EQ(ARCH_ARM, elf400_2->arch());
+  EXPECT_EQ(0x400U, info400_1.elf_offset);
+  EXPECT_EQ(0x400U, info400_2.elf_offset);
+  if (cache_enabled) {
+    EXPECT_EQ(elf400_1, elf400_2);
+    EXPECT_EQ(elf300_1, elf400_1);
+  } else {
+    EXPECT_NE(elf400_1, elf400_2);
+    EXPECT_NE(elf300_1, elf400_1);
+  }
+}
+
+TEST_F(ElfCacheTest, no_caching_valid_elf_offset_non_zero_never_read_at_zero) {
+  VerifyWithinSameMapNeverReadAtZero(false);
+}
+
+TEST_F(ElfCacheTest, caching_valid_elf_offset_non_zero_never_read_at_zero) {
+  VerifyWithinSameMapNeverReadAtZero(true);
+}
+
 }  // namespace unwindstack