Modify elf cache to handle elf_offsets properly.
Bug: 73498823
Test: All unit tests pass.
Test: Simpleperf run that previously failed, passes now.
Change-Id: Iff3a1f2f641a46ab9a0326579af3649f0c76fc65
diff --git a/libunwindstack/Elf.cpp b/libunwindstack/Elf.cpp
index dbf772e..02f8a9a 100644
--- a/libunwindstack/Elf.cpp
+++ b/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 @@
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::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;
diff --git a/libunwindstack/MapInfo.cpp b/libunwindstack/MapInfo.cpp
index 0c15335..39378a3 100644
--- a/libunwindstack/MapInfo.cpp
+++ b/libunwindstack/MapInfo.cpp
@@ -117,23 +117,15 @@
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();
diff --git a/libunwindstack/include/unwindstack/Elf.h b/libunwindstack/include/unwindstack/Elf.h
index a874709..385973e 100644
--- a/libunwindstack/include/unwindstack/Elf.h
+++ b/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 @@
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 @@
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_;
};
diff --git a/libunwindstack/tests/ElfCacheTest.cpp b/libunwindstack/tests/ElfCacheTest.cpp
index 0086c9e..89331ea 100644
--- a/libunwindstack/tests/ElfCacheTest.cpp
+++ b/libunwindstack/tests/ElfCacheTest.cpp
@@ -60,6 +60,7 @@
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 @@
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