From d9575b668bd122f1ad80767bc36024e4571ffeb0 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Fri, 16 Feb 2018 13:48:19 -0800 Subject: [PATCH] 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 --- libunwindstack/Elf.cpp | 55 +++++++++++++++++---- libunwindstack/MapInfo.cpp | 14 ++---- libunwindstack/include/unwindstack/Elf.h | 6 ++- libunwindstack/tests/ElfCacheTest.cpp | 63 ++++++++++++++++++++++++ 4 files changed, 116 insertions(+), 22 deletions(-) diff --git a/libunwindstack/Elf.cpp b/libunwindstack/Elf.cpp index dbf772e5d..02f8a9ae0 100644 --- a/libunwindstack/Elf.cpp +++ b/libunwindstack/Elf.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #define LOG_TAG "unwind" #include @@ -36,7 +37,7 @@ namespace unwindstack { bool Elf::cache_enabled_; -std::unordered_map>* Elf::cache_; +std::unordered_map, 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>; + cache_ = new std::unordered_map, 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) { +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 0c153351d..39378a3af 100644 --- a/libunwindstack/MapInfo.cpp +++ b/libunwindstack/MapInfo.cpp @@ -117,23 +117,15 @@ Elf* MapInfo::GetElf(const std::shared_ptr& 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(); diff --git a/libunwindstack/include/unwindstack/Elf.h b/libunwindstack/include/unwindstack/Elf.h index a874709d2..385973e9f 100644 --- a/libunwindstack/include/unwindstack/Elf.h +++ b/libunwindstack/include/unwindstack/Elf.h @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -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); + static bool CacheGet(MapInfo* info); + static bool CacheAfterCreateMemory(MapInfo* info); protected: bool valid_ = false; @@ -120,7 +122,7 @@ class Elf { std::unique_ptr gnu_debugdata_interface_; static bool cache_enabled_; - static std::unordered_map>* cache_; + static std::unordered_map, bool>>* cache_; static std::mutex* cache_lock_; }; diff --git a/libunwindstack/tests/ElfCacheTest.cpp b/libunwindstack/tests/ElfCacheTest.cpp index 0086c9edb..89331ea26 100644 --- a/libunwindstack/tests/ElfCacheTest.cpp +++ b/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_; }; @@ -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