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
diff --git a/libunwindstack/ElfInterface.cpp b/libunwindstack/ElfInterface.cpp
index 915cddb..2c00456 100644
--- a/libunwindstack/ElfInterface.cpp
+++ b/libunwindstack/ElfInterface.cpp
@@ -204,49 +204,19 @@
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<uintptr_t>(&phdr.p_type) - reinterpret_cast<uintptr_t>(&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<uintptr_t>(&phdr.p_flags) -
- reinterpret_cast<uintptr_t>(&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<uintptr_t>(&phdr.p_vaddr) -
- reinterpret_cast<uintptr_t>(&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<uintptr_t>(&phdr.p_offset) -
- reinterpret_cast<uintptr_t>(&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<uintptr_t>(&phdr.p_memsz) -
- reinterpret_cast<uintptr_t>(&phdr);
- return false;
- }
pt_loads_[phdr.p_offset] = LoadInfo{phdr.p_offset, phdr.p_vaddr,
static_cast<size_t>(phdr.p_memsz)};
if (phdr.p_offset == 0) {
@@ -256,46 +226,20 @@
}
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<uintptr_t>(&phdr.p_offset) -
- reinterpret_cast<uintptr_t>(&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<uintptr_t>(&phdr.p_memsz) -
- reinterpret_cast<uintptr_t>(&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<uintptr_t>(&phdr.p_offset) -
- reinterpret_cast<uintptr_t>(&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<uintptr_t>(&phdr.p_vaddr) -
- reinterpret_cast<uintptr_t>(&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<uintptr_t>(&phdr.p_memsz) -
- reinterpret_cast<uintptr_t>(&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 @@
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 a3244e8..3dd5d54 100644
--- a/libunwindstack/ElfInterfaceArm.cpp
+++ b/libunwindstack/ElfInterfaceArm.cpp
@@ -87,23 +87,17 @@
#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 3bee9cf..4c3a0c3 100644
--- a/libunwindstack/ElfInterfaceArm.h
+++ b/libunwindstack/ElfInterfaceArm.h
@@ -70,7 +70,7 @@
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 0c588da..5c1210d 100644
--- a/libunwindstack/include/unwindstack/ElfInterface.h
+++ b/libunwindstack/include/unwindstack/ElfInterface.h
@@ -118,7 +118,7 @@
template <typename SymType>
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 <typename EhdrType>
static void GetMaxSizeWithTemplate(Memory* memory, uint64_t* size);
diff --git a/libunwindstack/include/unwindstack/Memory.h b/libunwindstack/include/unwindstack/Memory.h
index c0c07f4..dee5e98 100644
--- a/libunwindstack/include/unwindstack/Memory.h
+++ b/libunwindstack/include/unwindstack/Memory.h
@@ -41,18 +41,6 @@
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<uintptr_t>(field) < reinterpret_cast<uintptr_t>(start)) {
- return false;
- }
- uint64_t offset = reinterpret_cast<uintptr_t>(field) - reinterpret_cast<uintptr_t>(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 a8bb4aa..43c6a97 100644
--- a/libunwindstack/tests/ElfInterfaceArmTest.cpp
+++ b/libunwindstack/tests/ElfInterfaceArmTest.cpp
@@ -242,44 +242,21 @@
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 4a9ed9f..3655984 100644
--- a/libunwindstack/tests/MemoryTest.cpp
+++ b/libunwindstack/tests/MemoryTest.cpp
@@ -51,40 +51,6 @@
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");