From 5afddb06370dc8b3d04a31f6f3ae04a5fa379cde Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Fri, 29 Jun 2018 16:30:55 -0700 Subject: [PATCH] Remove Memory::ReadField. In almost all cases, it is faster to read the entire structure rather than do multiple reads using ReadField. The only case where it would be slower is if doing a remote unwind and ptrace is the only way to read. In all other cases, it's a single system call. In the ptrace call, it will be multiple calls. Given that it is unusual to be forced to use ptrace, it's better to avoid it. It also reduces the code complexity to do a single read, and avoids issues where the code forgets to read the field it needs. Test: Unit tests pass on host and target. Change-Id: I7b3875b2c85d0d88115b1776e1be28521dc0b932 --- libunwindstack/ElfInterface.cpp | 71 ++----------------- libunwindstack/ElfInterfaceArm.cpp | 14 ++-- libunwindstack/ElfInterfaceArm.h | 2 +- .../include/unwindstack/ElfInterface.h | 2 +- libunwindstack/include/unwindstack/Memory.h | 12 ---- libunwindstack/tests/ElfInterfaceArmTest.cpp | 33 ++------- libunwindstack/tests/MemoryTest.cpp | 34 --------- 7 files changed, 18 insertions(+), 150 deletions(-) diff --git a/libunwindstack/ElfInterface.cpp b/libunwindstack/ElfInterface.cpp index 915cddb08..2c0045691 100644 --- a/libunwindstack/ElfInterface.cpp +++ b/libunwindstack/ElfInterface.cpp @@ -204,49 +204,19 @@ bool ElfInterface::ReadProgramHeaders(const EhdrType& ehdr, uint64_t* load_bias) uint64_t offset = ehdr.e_phoff; for (size_t i = 0; i < ehdr.e_phnum; i++, offset += ehdr.e_phentsize) { PhdrType phdr; - if (!memory_->ReadField(offset, &phdr, &phdr.p_type, sizeof(phdr.p_type))) { + if (!memory_->ReadFully(offset, &phdr, sizeof(phdr))) { last_error_.code = ERROR_MEMORY_INVALID; - last_error_.address = - offset + reinterpret_cast(&phdr.p_type) - reinterpret_cast(&phdr); + last_error_.address = offset; return false; } - if (HandleType(offset, phdr.p_type)) { - continue; - } - switch (phdr.p_type) { case PT_LOAD: { - // Get the flags first, if this isn't an executable header, ignore it. - if (!memory_->ReadField(offset, &phdr, &phdr.p_flags, sizeof(phdr.p_flags))) { - last_error_.code = ERROR_MEMORY_INVALID; - last_error_.address = offset + reinterpret_cast(&phdr.p_flags) - - reinterpret_cast(&phdr); - return false; - } if ((phdr.p_flags & PF_X) == 0) { continue; } - if (!memory_->ReadField(offset, &phdr, &phdr.p_vaddr, sizeof(phdr.p_vaddr))) { - last_error_.code = ERROR_MEMORY_INVALID; - last_error_.address = offset + reinterpret_cast(&phdr.p_vaddr) - - reinterpret_cast(&phdr); - return false; - } - if (!memory_->ReadField(offset, &phdr, &phdr.p_offset, sizeof(phdr.p_offset))) { - last_error_.code = ERROR_MEMORY_INVALID; - last_error_.address = offset + reinterpret_cast(&phdr.p_offset) - - reinterpret_cast(&phdr); - return false; - } - if (!memory_->ReadField(offset, &phdr, &phdr.p_memsz, sizeof(phdr.p_memsz))) { - last_error_.code = ERROR_MEMORY_INVALID; - last_error_.address = offset + reinterpret_cast(&phdr.p_memsz) - - reinterpret_cast(&phdr); - return false; - } pt_loads_[phdr.p_offset] = LoadInfo{phdr.p_offset, phdr.p_vaddr, static_cast(phdr.p_memsz)}; if (phdr.p_offset == 0) { @@ -256,46 +226,20 @@ bool ElfInterface::ReadProgramHeaders(const EhdrType& ehdr, uint64_t* load_bias) } case PT_GNU_EH_FRAME: - if (!memory_->ReadField(offset, &phdr, &phdr.p_offset, sizeof(phdr.p_offset))) { - last_error_.code = ERROR_MEMORY_INVALID; - last_error_.address = offset + reinterpret_cast(&phdr.p_offset) - - reinterpret_cast(&phdr); - return false; - } // This is really the pointer to the .eh_frame_hdr section. eh_frame_hdr_offset_ = phdr.p_offset; - if (!memory_->ReadField(offset, &phdr, &phdr.p_memsz, sizeof(phdr.p_memsz))) { - last_error_.code = ERROR_MEMORY_INVALID; - last_error_.address = offset + reinterpret_cast(&phdr.p_memsz) - - reinterpret_cast(&phdr); - return false; - } eh_frame_hdr_size_ = phdr.p_memsz; break; case PT_DYNAMIC: - if (!memory_->ReadField(offset, &phdr, &phdr.p_offset, sizeof(phdr.p_offset))) { - last_error_.code = ERROR_MEMORY_INVALID; - last_error_.address = offset + reinterpret_cast(&phdr.p_offset) - - reinterpret_cast(&phdr); - return false; - } dynamic_offset_ = phdr.p_offset; - if (!memory_->ReadField(offset, &phdr, &phdr.p_vaddr, sizeof(phdr.p_vaddr))) { - last_error_.code = ERROR_MEMORY_INVALID; - last_error_.address = offset + reinterpret_cast(&phdr.p_vaddr) - - reinterpret_cast(&phdr); - return false; - } dynamic_vaddr_ = phdr.p_vaddr; - if (!memory_->ReadField(offset, &phdr, &phdr.p_memsz, sizeof(phdr.p_memsz))) { - last_error_.code = ERROR_MEMORY_INVALID; - last_error_.address = offset + reinterpret_cast(&phdr.p_memsz) - - reinterpret_cast(&phdr); - return false; - } dynamic_size_ = phdr.p_memsz; break; + + default: + HandleUnknownType(phdr.p_type, phdr.p_offset, phdr.p_filesz); + break; } } return true; @@ -313,8 +257,7 @@ bool ElfInterface::ReadSectionHeaders(const EhdrType& ehdr) { ShdrType shdr; if (ehdr.e_shstrndx < ehdr.e_shnum) { uint64_t sh_offset = offset + ehdr.e_shstrndx * ehdr.e_shentsize; - if (memory_->ReadField(sh_offset, &shdr, &shdr.sh_offset, sizeof(shdr.sh_offset)) && - memory_->ReadField(sh_offset, &shdr, &shdr.sh_size, sizeof(shdr.sh_size))) { + if (memory_->ReadFully(sh_offset, &shdr, sizeof(shdr))) { sec_offset = shdr.sh_offset; sec_size = shdr.sh_size; } diff --git a/libunwindstack/ElfInterfaceArm.cpp b/libunwindstack/ElfInterfaceArm.cpp index a3244e824..3dd5d54fe 100644 --- a/libunwindstack/ElfInterfaceArm.cpp +++ b/libunwindstack/ElfInterfaceArm.cpp @@ -87,23 +87,17 @@ bool ElfInterfaceArm::GetPrel31Addr(uint32_t offset, uint32_t* addr) { #define PT_ARM_EXIDX 0x70000001 #endif -bool ElfInterfaceArm::HandleType(uint64_t offset, uint32_t type) { +void ElfInterfaceArm::HandleUnknownType(uint32_t type, uint64_t ph_offset, uint64_t ph_filesz) { if (type != PT_ARM_EXIDX) { - return false; - } - - Elf32_Phdr phdr; - if (!memory_->ReadFully(offset, &phdr, sizeof(phdr))) { - return true; + return; } // The offset already takes into account the load bias. - start_offset_ = phdr.p_offset; + start_offset_ = ph_offset; // Always use filesz instead of memsz. In most cases they are the same, // but some shared libraries wind up setting one correctly and not the other. - total_entries_ = phdr.p_filesz / 8; - return true; + total_entries_ = ph_filesz / 8; } bool ElfInterfaceArm::Step(uint64_t pc, Regs* regs, Memory* process_memory, bool* finished) { diff --git a/libunwindstack/ElfInterfaceArm.h b/libunwindstack/ElfInterfaceArm.h index 3bee9cff1..4c3a0c347 100644 --- a/libunwindstack/ElfInterfaceArm.h +++ b/libunwindstack/ElfInterfaceArm.h @@ -70,7 +70,7 @@ class ElfInterfaceArm : public ElfInterface32 { bool FindEntry(uint32_t pc, uint64_t* entry_offset); - bool HandleType(uint64_t offset, uint32_t type) override; + void HandleUnknownType(uint32_t type, uint64_t ph_offset, uint64_t ph_filesz) override; bool Step(uint64_t pc, Regs* regs, Memory* process_memory, bool* finished) override; diff --git a/libunwindstack/include/unwindstack/ElfInterface.h b/libunwindstack/include/unwindstack/ElfInterface.h index 0c588da7d..5c1210d68 100644 --- a/libunwindstack/include/unwindstack/ElfInterface.h +++ b/libunwindstack/include/unwindstack/ElfInterface.h @@ -118,7 +118,7 @@ class ElfInterface { template bool GetGlobalVariableWithTemplate(const std::string& name, uint64_t* memory_address); - virtual bool HandleType(uint64_t, uint32_t) { return false; } + virtual void HandleUnknownType(uint32_t, uint64_t, uint64_t) {} template static void GetMaxSizeWithTemplate(Memory* memory, uint64_t* size); diff --git a/libunwindstack/include/unwindstack/Memory.h b/libunwindstack/include/unwindstack/Memory.h index c0c07f4f9..dee5e983c 100644 --- a/libunwindstack/include/unwindstack/Memory.h +++ b/libunwindstack/include/unwindstack/Memory.h @@ -41,18 +41,6 @@ class Memory { bool ReadFully(uint64_t addr, void* dst, size_t size); - inline bool ReadField(uint64_t addr, void* start, void* field, size_t size) { - if (reinterpret_cast(field) < reinterpret_cast(start)) { - return false; - } - uint64_t offset = reinterpret_cast(field) - reinterpret_cast(start); - if (__builtin_add_overflow(addr, offset, &offset)) { - return false; - } - // The read will check if offset + size overflows. - return ReadFully(offset, field, size); - } - inline bool Read32(uint64_t addr, uint32_t* dst) { return ReadFully(addr, dst, sizeof(uint32_t)); } diff --git a/libunwindstack/tests/ElfInterfaceArmTest.cpp b/libunwindstack/tests/ElfInterfaceArmTest.cpp index a8bb4aaf4..43c6a9734 100644 --- a/libunwindstack/tests/ElfInterfaceArmTest.cpp +++ b/libunwindstack/tests/ElfInterfaceArmTest.cpp @@ -242,44 +242,21 @@ TEST_F(ElfInterfaceArmTest, iterate) { ASSERT_EQ(0xa020U, entries[4]); } -TEST_F(ElfInterfaceArmTest, HandleType_not_arm_exidx) { +TEST_F(ElfInterfaceArmTest, HandleUnknownType_arm_exidx) { ElfInterfaceArmFake interface(&memory_); - ASSERT_FALSE(interface.HandleType(0x1000, PT_NULL)); - ASSERT_FALSE(interface.HandleType(0x1000, PT_LOAD)); - ASSERT_FALSE(interface.HandleType(0x1000, PT_DYNAMIC)); - ASSERT_FALSE(interface.HandleType(0x1000, PT_INTERP)); - ASSERT_FALSE(interface.HandleType(0x1000, PT_NOTE)); - ASSERT_FALSE(interface.HandleType(0x1000, PT_SHLIB)); - ASSERT_FALSE(interface.HandleType(0x1000, PT_PHDR)); - ASSERT_FALSE(interface.HandleType(0x1000, PT_TLS)); - ASSERT_FALSE(interface.HandleType(0x1000, PT_LOOS)); - ASSERT_FALSE(interface.HandleType(0x1000, PT_HIOS)); - ASSERT_FALSE(interface.HandleType(0x1000, PT_LOPROC)); - ASSERT_FALSE(interface.HandleType(0x1000, PT_HIPROC)); - ASSERT_FALSE(interface.HandleType(0x1000, PT_GNU_EH_FRAME)); - ASSERT_FALSE(interface.HandleType(0x1000, PT_GNU_STACK)); -} - -TEST_F(ElfInterfaceArmTest, HandleType_arm_exidx) { - ElfInterfaceArmFake interface(&memory_); - - Elf32_Phdr phdr = {}; interface.FakeSetStartOffset(0x1000); interface.FakeSetTotalEntries(100); - phdr.p_offset = 0x2000; - phdr.p_filesz = 0xa00; - // Verify that if reads fail, we don't set the values but still get true. - ASSERT_TRUE(interface.HandleType(0x1000, 0x70000001)); + // Verify that if the type is not the one we want, we don't set the values. + interface.HandleUnknownType(0x70000000, 0x2000, 320); ASSERT_EQ(0x1000U, interface.start_offset()); ASSERT_EQ(100U, interface.total_entries()); // Everything is correct and present. - memory_.SetMemory(0x1000, &phdr, sizeof(phdr)); - ASSERT_TRUE(interface.HandleType(0x1000, 0x70000001)); + interface.HandleUnknownType(0x70000001, 0x2000, 320); ASSERT_EQ(0x2000U, interface.start_offset()); - ASSERT_EQ(320U, interface.total_entries()); + ASSERT_EQ(40U, interface.total_entries()); } TEST_F(ElfInterfaceArmTest, StepExidx) { diff --git a/libunwindstack/tests/MemoryTest.cpp b/libunwindstack/tests/MemoryTest.cpp index 4a9ed9f58..365598499 100644 --- a/libunwindstack/tests/MemoryTest.cpp +++ b/libunwindstack/tests/MemoryTest.cpp @@ -51,40 +51,6 @@ struct FakeStruct { uint64_t four; }; -TEST(MemoryTest, read_field) { - MemoryFakeAlwaysReadZero memory; - - FakeStruct data; - memset(&data, 0xff, sizeof(data)); - ASSERT_TRUE(memory.ReadField(0, &data, &data.one, sizeof(data.one))); - ASSERT_EQ(0, data.one); - - memset(&data, 0xff, sizeof(data)); - ASSERT_TRUE(memory.ReadField(0, &data, &data.two, sizeof(data.two))); - ASSERT_FALSE(data.two); - - memset(&data, 0xff, sizeof(data)); - ASSERT_TRUE(memory.ReadField(0, &data, &data.three, sizeof(data.three))); - ASSERT_EQ(0U, data.three); - - memset(&data, 0xff, sizeof(data)); - ASSERT_TRUE(memory.ReadField(0, &data, &data.four, sizeof(data.four))); - ASSERT_EQ(0U, data.four); -} - -TEST(MemoryTest, read_field_fails) { - MemoryFakeAlwaysReadZero memory; - - FakeStruct data; - memset(&data, 0xff, sizeof(data)); - - ASSERT_FALSE(memory.ReadField(UINT64_MAX, &data, &data.three, sizeof(data.three))); - - // Field and start reversed, should fail. - ASSERT_FALSE(memory.ReadField(100, &data.two, &data, sizeof(data.two))); - ASSERT_FALSE(memory.ReadField(0, &data.two, &data, sizeof(data.two))); -} - TEST(MemoryTest, read_string) { std::string name("string_in_memory");