From 4b81f63bbf0180882c63356083c004e1fbd91ec9 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Wed, 28 Mar 2018 15:12:49 -0700 Subject: Fix null pointer dereference in RegsArm. Fix RegsArm::GetPcAdjustment to check for an invalid elf before trying to read memory. Modify the tests for this so it crashes without this change. Also modify the GetPcAdjustment for all different architectures so that unless the relative pc is too small, it will return the minimum amount that should be adjusted. This is to handle cases where we still want to adjust the pc but it's in an invalid elf. Mostly this is for handling cases when the pc is in jit gdb debug code so that we use the right unwind information. Bug: 77233204 Test: Passes unit tests for libbacktrace/libunwindstack. Change-Id: Id73609adaf3b80a583584441de228156fec3afa7 (cherry picked from commit 6dbc28ece3ab7cadd0087b4dc31ba9a2986545f0) --- libunwindstack/RegsArm.cpp | 14 +++- libunwindstack/RegsArm64.cpp | 4 +- libunwindstack/RegsMips.cpp | 4 +- libunwindstack/RegsMips64.cpp | 4 +- libunwindstack/RegsX86.cpp | 4 +- libunwindstack/RegsX86_64.cpp | 4 +- libunwindstack/tests/RegsTest.cpp | 119 +++++++++++++++-------------- libunwindstack/tests/UnwindOfflineTest.cpp | 32 ++++---- 8 files changed, 99 insertions(+), 86 deletions(-) diff --git a/libunwindstack/RegsArm.cpp b/libunwindstack/RegsArm.cpp index e2a9cb0..27cab43 100644 --- a/libunwindstack/RegsArm.cpp +++ b/libunwindstack/RegsArm.cpp @@ -51,13 +51,23 @@ void RegsArm::set_sp(uint64_t sp) { } uint64_t RegsArm::GetPcAdjustment(uint64_t rel_pc, Elf* elf) { + if (!elf->valid()) { + return 2; + } + uint64_t load_bias = elf->GetLoadBias(); if (rel_pc < load_bias) { - return 0; + if (rel_pc < 2) { + return 0; + } + return 2; } uint64_t adjusted_rel_pc = rel_pc - load_bias; if (adjusted_rel_pc < 5) { - return 0; + if (adjusted_rel_pc < 2) { + return 0; + } + return 2; } if (adjusted_rel_pc & 1) { diff --git a/libunwindstack/RegsArm64.cpp b/libunwindstack/RegsArm64.cpp index fe24c80..4a2a6c4 100644 --- a/libunwindstack/RegsArm64.cpp +++ b/libunwindstack/RegsArm64.cpp @@ -51,8 +51,8 @@ void RegsArm64::set_sp(uint64_t sp) { regs_[ARM64_REG_SP] = sp; } -uint64_t RegsArm64::GetPcAdjustment(uint64_t rel_pc, Elf* elf) { - if (!elf->valid() || rel_pc < 4) { +uint64_t RegsArm64::GetPcAdjustment(uint64_t rel_pc, Elf*) { + if (rel_pc < 4) { return 0; } return 4; diff --git a/libunwindstack/RegsMips.cpp b/libunwindstack/RegsMips.cpp index 0b10e21..c87e69b 100644 --- a/libunwindstack/RegsMips.cpp +++ b/libunwindstack/RegsMips.cpp @@ -51,8 +51,8 @@ void RegsMips::set_sp(uint64_t sp) { regs_[MIPS_REG_SP] = static_cast(sp); } -uint64_t RegsMips::GetPcAdjustment(uint64_t rel_pc, Elf* elf) { - if (!elf->valid() || rel_pc < 8) { +uint64_t RegsMips::GetPcAdjustment(uint64_t rel_pc, Elf*) { + if (rel_pc < 8) { return 0; } // For now, just assume no compact branches diff --git a/libunwindstack/RegsMips64.cpp b/libunwindstack/RegsMips64.cpp index 8848e3b..236a922 100644 --- a/libunwindstack/RegsMips64.cpp +++ b/libunwindstack/RegsMips64.cpp @@ -51,8 +51,8 @@ void RegsMips64::set_sp(uint64_t sp) { regs_[MIPS64_REG_SP] = sp; } -uint64_t RegsMips64::GetPcAdjustment(uint64_t rel_pc, Elf* elf) { - if (!elf->valid() || rel_pc < 8) { +uint64_t RegsMips64::GetPcAdjustment(uint64_t rel_pc, Elf*) { + if (rel_pc < 8) { return 0; } // For now, just assume no compact branches diff --git a/libunwindstack/RegsX86.cpp b/libunwindstack/RegsX86.cpp index bb95a13..f7e0614 100644 --- a/libunwindstack/RegsX86.cpp +++ b/libunwindstack/RegsX86.cpp @@ -50,8 +50,8 @@ void RegsX86::set_sp(uint64_t sp) { regs_[X86_REG_SP] = static_cast(sp); } -uint64_t RegsX86::GetPcAdjustment(uint64_t rel_pc, Elf* elf) { - if (!elf->valid() || rel_pc == 0) { +uint64_t RegsX86::GetPcAdjustment(uint64_t rel_pc, Elf*) { + if (rel_pc == 0) { return 0; } return 1; diff --git a/libunwindstack/RegsX86_64.cpp b/libunwindstack/RegsX86_64.cpp index e57e2bc..7d6ad86 100644 --- a/libunwindstack/RegsX86_64.cpp +++ b/libunwindstack/RegsX86_64.cpp @@ -50,8 +50,8 @@ void RegsX86_64::set_sp(uint64_t sp) { regs_[X86_64_REG_SP] = sp; } -uint64_t RegsX86_64::GetPcAdjustment(uint64_t rel_pc, Elf* elf) { - if (!elf->valid() || rel_pc == 0) { +uint64_t RegsX86_64::GetPcAdjustment(uint64_t rel_pc, Elf*) { + if (rel_pc == 0) { return 0; } return 1; diff --git a/libunwindstack/tests/RegsTest.cpp b/libunwindstack/tests/RegsTest.cpp index 3e80733..d15823e 100644 --- a/libunwindstack/tests/RegsTest.cpp +++ b/libunwindstack/tests/RegsTest.cpp @@ -94,48 +94,48 @@ TEST_F(RegsTest, regs64) { TEST_F(RegsTest, rel_pc) { RegsArm64 arm64; - ASSERT_EQ(4U, arm64.GetPcAdjustment(0x10, elf_.get())); - ASSERT_EQ(4U, arm64.GetPcAdjustment(0x4, elf_.get())); - ASSERT_EQ(0U, arm64.GetPcAdjustment(0x3, elf_.get())); - ASSERT_EQ(0U, arm64.GetPcAdjustment(0x2, elf_.get())); - ASSERT_EQ(0U, arm64.GetPcAdjustment(0x1, elf_.get())); - ASSERT_EQ(0U, arm64.GetPcAdjustment(0x0, elf_.get())); + EXPECT_EQ(4U, arm64.GetPcAdjustment(0x10, elf_.get())); + EXPECT_EQ(4U, arm64.GetPcAdjustment(0x4, elf_.get())); + EXPECT_EQ(0U, arm64.GetPcAdjustment(0x3, elf_.get())); + EXPECT_EQ(0U, arm64.GetPcAdjustment(0x2, elf_.get())); + EXPECT_EQ(0U, arm64.GetPcAdjustment(0x1, elf_.get())); + EXPECT_EQ(0U, arm64.GetPcAdjustment(0x0, elf_.get())); RegsX86 x86; - ASSERT_EQ(1U, x86.GetPcAdjustment(0x100, elf_.get())); - ASSERT_EQ(1U, x86.GetPcAdjustment(0x2, elf_.get())); - ASSERT_EQ(1U, x86.GetPcAdjustment(0x1, elf_.get())); - ASSERT_EQ(0U, x86.GetPcAdjustment(0x0, elf_.get())); + EXPECT_EQ(1U, x86.GetPcAdjustment(0x100, elf_.get())); + EXPECT_EQ(1U, x86.GetPcAdjustment(0x2, elf_.get())); + EXPECT_EQ(1U, x86.GetPcAdjustment(0x1, elf_.get())); + EXPECT_EQ(0U, x86.GetPcAdjustment(0x0, elf_.get())); RegsX86_64 x86_64; - ASSERT_EQ(1U, x86_64.GetPcAdjustment(0x100, elf_.get())); - ASSERT_EQ(1U, x86_64.GetPcAdjustment(0x2, elf_.get())); - ASSERT_EQ(1U, x86_64.GetPcAdjustment(0x1, elf_.get())); - ASSERT_EQ(0U, x86_64.GetPcAdjustment(0x0, elf_.get())); + EXPECT_EQ(1U, x86_64.GetPcAdjustment(0x100, elf_.get())); + EXPECT_EQ(1U, x86_64.GetPcAdjustment(0x2, elf_.get())); + EXPECT_EQ(1U, x86_64.GetPcAdjustment(0x1, elf_.get())); + EXPECT_EQ(0U, x86_64.GetPcAdjustment(0x0, elf_.get())); RegsMips mips; - ASSERT_EQ(8U, mips.GetPcAdjustment(0x10, elf_.get())); - ASSERT_EQ(8U, mips.GetPcAdjustment(0x8, elf_.get())); - ASSERT_EQ(0U, mips.GetPcAdjustment(0x7, elf_.get())); - ASSERT_EQ(0U, mips.GetPcAdjustment(0x6, elf_.get())); - ASSERT_EQ(0U, mips.GetPcAdjustment(0x5, elf_.get())); - ASSERT_EQ(0U, mips.GetPcAdjustment(0x4, elf_.get())); - ASSERT_EQ(0U, mips.GetPcAdjustment(0x3, elf_.get())); - ASSERT_EQ(0U, mips.GetPcAdjustment(0x2, elf_.get())); - ASSERT_EQ(0U, mips.GetPcAdjustment(0x1, elf_.get())); - ASSERT_EQ(0U, mips.GetPcAdjustment(0x0, elf_.get())); + EXPECT_EQ(8U, mips.GetPcAdjustment(0x10, elf_.get())); + EXPECT_EQ(8U, mips.GetPcAdjustment(0x8, elf_.get())); + EXPECT_EQ(0U, mips.GetPcAdjustment(0x7, elf_.get())); + EXPECT_EQ(0U, mips.GetPcAdjustment(0x6, elf_.get())); + EXPECT_EQ(0U, mips.GetPcAdjustment(0x5, elf_.get())); + EXPECT_EQ(0U, mips.GetPcAdjustment(0x4, elf_.get())); + EXPECT_EQ(0U, mips.GetPcAdjustment(0x3, elf_.get())); + EXPECT_EQ(0U, mips.GetPcAdjustment(0x2, elf_.get())); + EXPECT_EQ(0U, mips.GetPcAdjustment(0x1, elf_.get())); + EXPECT_EQ(0U, mips.GetPcAdjustment(0x0, elf_.get())); RegsMips64 mips64; - ASSERT_EQ(8U, mips64.GetPcAdjustment(0x10, elf_.get())); - ASSERT_EQ(8U, mips64.GetPcAdjustment(0x8, elf_.get())); - ASSERT_EQ(0U, mips64.GetPcAdjustment(0x7, elf_.get())); - ASSERT_EQ(0U, mips64.GetPcAdjustment(0x6, elf_.get())); - ASSERT_EQ(0U, mips64.GetPcAdjustment(0x5, elf_.get())); - ASSERT_EQ(0U, mips64.GetPcAdjustment(0x4, elf_.get())); - ASSERT_EQ(0U, mips64.GetPcAdjustment(0x3, elf_.get())); - ASSERT_EQ(0U, mips64.GetPcAdjustment(0x2, elf_.get())); - ASSERT_EQ(0U, mips64.GetPcAdjustment(0x1, elf_.get())); - ASSERT_EQ(0U, mips64.GetPcAdjustment(0x0, elf_.get())); + EXPECT_EQ(8U, mips64.GetPcAdjustment(0x10, elf_.get())); + EXPECT_EQ(8U, mips64.GetPcAdjustment(0x8, elf_.get())); + EXPECT_EQ(0U, mips64.GetPcAdjustment(0x7, elf_.get())); + EXPECT_EQ(0U, mips64.GetPcAdjustment(0x6, elf_.get())); + EXPECT_EQ(0U, mips64.GetPcAdjustment(0x5, elf_.get())); + EXPECT_EQ(0U, mips64.GetPcAdjustment(0x4, elf_.get())); + EXPECT_EQ(0U, mips64.GetPcAdjustment(0x3, elf_.get())); + EXPECT_EQ(0U, mips64.GetPcAdjustment(0x2, elf_.get())); + EXPECT_EQ(0U, mips64.GetPcAdjustment(0x1, elf_.get())); + EXPECT_EQ(0U, mips64.GetPcAdjustment(0x0, elf_.get())); } TEST_F(RegsTest, rel_pc_arm) { @@ -143,34 +143,36 @@ TEST_F(RegsTest, rel_pc_arm) { // Check fence posts. elf_->FakeSetLoadBias(0); - ASSERT_EQ(2U, arm.GetPcAdjustment(0x5, elf_.get())); - ASSERT_EQ(0U, arm.GetPcAdjustment(0x4, elf_.get())); - ASSERT_EQ(0U, arm.GetPcAdjustment(0x3, elf_.get())); - ASSERT_EQ(0U, arm.GetPcAdjustment(0x2, elf_.get())); - ASSERT_EQ(0U, arm.GetPcAdjustment(0x1, elf_.get())); - ASSERT_EQ(0U, arm.GetPcAdjustment(0x0, elf_.get())); + EXPECT_EQ(2U, arm.GetPcAdjustment(0x5, elf_.get())); + EXPECT_EQ(2U, arm.GetPcAdjustment(0x4, elf_.get())); + EXPECT_EQ(2U, arm.GetPcAdjustment(0x3, elf_.get())); + EXPECT_EQ(2U, arm.GetPcAdjustment(0x2, elf_.get())); + EXPECT_EQ(0U, arm.GetPcAdjustment(0x1, elf_.get())); + EXPECT_EQ(0U, arm.GetPcAdjustment(0x0, elf_.get())); elf_->FakeSetLoadBias(0x100); - ASSERT_EQ(0U, arm.GetPcAdjustment(0xff, elf_.get())); - ASSERT_EQ(2U, arm.GetPcAdjustment(0x105, elf_.get())); - ASSERT_EQ(0U, arm.GetPcAdjustment(0x104, elf_.get())); - ASSERT_EQ(0U, arm.GetPcAdjustment(0x103, elf_.get())); - ASSERT_EQ(0U, arm.GetPcAdjustment(0x102, elf_.get())); - ASSERT_EQ(0U, arm.GetPcAdjustment(0x101, elf_.get())); - ASSERT_EQ(0U, arm.GetPcAdjustment(0x100, elf_.get())); + EXPECT_EQ(0U, arm.GetPcAdjustment(0x1, elf_.get())); + EXPECT_EQ(2U, arm.GetPcAdjustment(0x2, elf_.get())); + EXPECT_EQ(2U, arm.GetPcAdjustment(0xff, elf_.get())); + EXPECT_EQ(2U, arm.GetPcAdjustment(0x105, elf_.get())); + EXPECT_EQ(2U, arm.GetPcAdjustment(0x104, elf_.get())); + EXPECT_EQ(2U, arm.GetPcAdjustment(0x103, elf_.get())); + EXPECT_EQ(2U, arm.GetPcAdjustment(0x102, elf_.get())); + EXPECT_EQ(0U, arm.GetPcAdjustment(0x101, elf_.get())); + EXPECT_EQ(0U, arm.GetPcAdjustment(0x100, elf_.get())); // Check thumb instructions handling. elf_->FakeSetLoadBias(0); memory_->SetData32(0x2000, 0); - ASSERT_EQ(2U, arm.GetPcAdjustment(0x2005, elf_.get())); + EXPECT_EQ(2U, arm.GetPcAdjustment(0x2005, elf_.get())); memory_->SetData32(0x2000, 0xe000f000); - ASSERT_EQ(4U, arm.GetPcAdjustment(0x2005, elf_.get())); + EXPECT_EQ(4U, arm.GetPcAdjustment(0x2005, elf_.get())); elf_->FakeSetLoadBias(0x400); memory_->SetData32(0x2100, 0); - ASSERT_EQ(2U, arm.GetPcAdjustment(0x2505, elf_.get())); + EXPECT_EQ(2U, arm.GetPcAdjustment(0x2505, elf_.get())); memory_->SetData32(0x2100, 0xf111f111); - ASSERT_EQ(4U, arm.GetPcAdjustment(0x2505, elf_.get())); + EXPECT_EQ(4U, arm.GetPcAdjustment(0x2505, elf_.get())); } TEST_F(RegsTest, elf_invalid) { @@ -181,32 +183,33 @@ TEST_F(RegsTest, elf_invalid) { RegsMips regs_mips; RegsMips64 regs_mips64; MapInfo map_info(0x1000, 0x2000); - Elf* invalid_elf = new Elf(new MemoryFake); + Elf* invalid_elf = new Elf(nullptr); map_info.elf.reset(invalid_elf); regs_arm.set_pc(0x1500); EXPECT_EQ(0x500U, invalid_elf->GetRelPc(regs_arm.pc(), &map_info)); - EXPECT_EQ(4U, regs_arm.GetPcAdjustment(0x500U, invalid_elf)); + EXPECT_EQ(2U, regs_arm.GetPcAdjustment(0x500U, invalid_elf)); + EXPECT_EQ(2U, regs_arm.GetPcAdjustment(0x511U, invalid_elf)); regs_arm64.set_pc(0x1600); EXPECT_EQ(0x600U, invalid_elf->GetRelPc(regs_arm64.pc(), &map_info)); - EXPECT_EQ(0U, regs_arm64.GetPcAdjustment(0x600U, invalid_elf)); + EXPECT_EQ(4U, regs_arm64.GetPcAdjustment(0x600U, invalid_elf)); regs_x86.set_pc(0x1700); EXPECT_EQ(0x700U, invalid_elf->GetRelPc(regs_x86.pc(), &map_info)); - EXPECT_EQ(0U, regs_x86.GetPcAdjustment(0x700U, invalid_elf)); + EXPECT_EQ(1U, regs_x86.GetPcAdjustment(0x700U, invalid_elf)); regs_x86_64.set_pc(0x1800); EXPECT_EQ(0x800U, invalid_elf->GetRelPc(regs_x86_64.pc(), &map_info)); - EXPECT_EQ(0U, regs_x86_64.GetPcAdjustment(0x800U, invalid_elf)); + EXPECT_EQ(1U, regs_x86_64.GetPcAdjustment(0x800U, invalid_elf)); regs_mips.set_pc(0x1900); EXPECT_EQ(0x900U, invalid_elf->GetRelPc(regs_mips.pc(), &map_info)); - EXPECT_EQ(0U, regs_mips.GetPcAdjustment(0x900U, invalid_elf)); + EXPECT_EQ(8U, regs_mips.GetPcAdjustment(0x900U, invalid_elf)); regs_mips64.set_pc(0x1a00); EXPECT_EQ(0xa00U, invalid_elf->GetRelPc(regs_mips64.pc(), &map_info)); - EXPECT_EQ(0U, regs_mips64.GetPcAdjustment(0xa00U, invalid_elf)); + EXPECT_EQ(8U, regs_mips64.GetPcAdjustment(0xa00U, invalid_elf)); } TEST_F(RegsTest, arm_verify_sp_pc) { diff --git a/libunwindstack/tests/UnwindOfflineTest.cpp b/libunwindstack/tests/UnwindOfflineTest.cpp index 532640f..8cb640f 100644 --- a/libunwindstack/tests/UnwindOfflineTest.cpp +++ b/libunwindstack/tests/UnwindOfflineTest.cpp @@ -284,7 +284,7 @@ TEST_F(UnwindOfflineTest, jit_debug_x86) { " #01 pc 00067f00 libarttestd.so (Java_Main_unwindInProcess+10032)\n" " #02 pc 000021a8 (offset 0x2000) 137-cfi.odex (boolean Main.unwindInProcess(boolean, int, " "boolean)+136)\n" - " #03 pc 0000fe81 anonymous:ee74c000 (boolean Main.bar(boolean)+65)\n" + " #03 pc 0000fe80 anonymous:ee74c000 (boolean Main.bar(boolean)+64)\n" " #04 pc 006ad4d2 libartd.so (art_quick_invoke_stub+338)\n" " #05 pc 00146ab5 libartd.so " "(_ZN3art9ArtMethod6InvokeEPNS_6ThreadEPjjPNS_6JValueEPKc+885)\n" @@ -299,7 +299,7 @@ TEST_F(UnwindOfflineTest, jit_debug_x86) { "20CodeItemDataAccessorEPNS_11ShadowFrameE+234)\n" " #09 pc 00684362 libartd.so (artQuickToInterpreterBridge+1058)\n" " #10 pc 006b35bd libartd.so (art_quick_to_interpreter_bridge+77)\n" - " #11 pc 0000fe04 anonymous:ee74c000 (int Main.compare(Main, Main)+52)\n" + " #11 pc 0000fe03 anonymous:ee74c000 (int Main.compare(Main, Main)+51)\n" " #12 pc 006ad4d2 libartd.so (art_quick_invoke_stub+338)\n" " #13 pc 00146ab5 libartd.so " "(_ZN3art9ArtMethod6InvokeEPNS_6ThreadEPjjPNS_6JValueEPKc+885)\n" @@ -314,8 +314,8 @@ TEST_F(UnwindOfflineTest, jit_debug_x86) { "20CodeItemDataAccessorEPNS_11ShadowFrameE+234)\n" " #17 pc 00684362 libartd.so (artQuickToInterpreterBridge+1058)\n" " #18 pc 006b35bd libartd.so (art_quick_to_interpreter_bridge+77)\n" - " #19 pc 0000fd3c anonymous:ee74c000 (int Main.compare(java.lang.Object, " - "java.lang.Object)+108)\n" + " #19 pc 0000fd3b anonymous:ee74c000 (int Main.compare(java.lang.Object, " + "java.lang.Object)+107)\n" " #20 pc 006ad4d2 libartd.so (art_quick_invoke_stub+338)\n" " #21 pc 00146ab5 libartd.so " "(_ZN3art9ArtMethod6InvokeEPNS_6ThreadEPjjPNS_6JValueEPKc+885)\n" @@ -330,9 +330,9 @@ TEST_F(UnwindOfflineTest, jit_debug_x86) { "20CodeItemDataAccessorEPNS_11ShadowFrameE+234)\n" " #25 pc 00684362 libartd.so (artQuickToInterpreterBridge+1058)\n" " #26 pc 006b35bd libartd.so (art_quick_to_interpreter_bridge+77)\n" - " #27 pc 0000fbdc anonymous:ee74c000 (int " + " #27 pc 0000fbdb anonymous:ee74c000 (int " "java.util.Arrays.binarySearch0(java.lang.Object[], int, int, java.lang.Object, " - "java.util.Comparator)+332)\n" + "java.util.Comparator)+331)\n" " #28 pc 006ad6a2 libartd.so (art_quick_invoke_static_stub+418)\n" " #29 pc 00146acb libartd.so " "(_ZN3art9ArtMethod6InvokeEPNS_6ThreadEPjjPNS_6JValueEPKc+907)\n" @@ -347,7 +347,7 @@ TEST_F(UnwindOfflineTest, jit_debug_x86) { "20CodeItemDataAccessorEPNS_11ShadowFrameE+234)\n" " #33 pc 00684362 libartd.so (artQuickToInterpreterBridge+1058)\n" " #34 pc 006b35bd libartd.so (art_quick_to_interpreter_bridge+77)\n" - " #35 pc 0000f625 anonymous:ee74c000 (boolean Main.foo()+165)\n" + " #35 pc 0000f624 anonymous:ee74c000 (boolean Main.foo()+164)\n" " #36 pc 006ad4d2 libartd.so (art_quick_invoke_stub+338)\n" " #37 pc 00146ab5 libartd.so " "(_ZN3art9ArtMethod6InvokeEPNS_6ThreadEPjjPNS_6JValueEPKc+885)\n" @@ -362,7 +362,7 @@ TEST_F(UnwindOfflineTest, jit_debug_x86) { "20CodeItemDataAccessorEPNS_11ShadowFrameE+234)\n" " #41 pc 00684362 libartd.so (artQuickToInterpreterBridge+1058)\n" " #42 pc 006b35bd libartd.so (art_quick_to_interpreter_bridge+77)\n" - " #43 pc 0000eedc anonymous:ee74c000 (void Main.runPrimary()+60)\n" + " #43 pc 0000eedb anonymous:ee74c000 (void Main.runPrimary()+59)\n" " #44 pc 006ad4d2 libartd.so (art_quick_invoke_stub+338)\n" " #45 pc 00146ab5 libartd.so " "(_ZN3art9ArtMethod6InvokeEPNS_6ThreadEPjjPNS_6JValueEPKc+885)\n" @@ -377,7 +377,7 @@ TEST_F(UnwindOfflineTest, jit_debug_x86) { "20CodeItemDataAccessorEPNS_11ShadowFrameE+234)\n" " #49 pc 00684362 libartd.so (artQuickToInterpreterBridge+1058)\n" " #50 pc 006b35bd libartd.so (art_quick_to_interpreter_bridge+77)\n" - " #51 pc 0000ac22 anonymous:ee74c000 (void Main.main(java.lang.String[])+98)\n" + " #51 pc 0000ac21 anonymous:ee74c000 (void Main.main(java.lang.String[])+97)\n" " #52 pc 006ad6a2 libartd.so (art_quick_invoke_static_stub+418)\n" " #53 pc 00146acb libartd.so " "(_ZN3art9ArtMethod6InvokeEPNS_6ThreadEPjjPNS_6JValueEPKc+907)\n" @@ -419,7 +419,7 @@ TEST_F(UnwindOfflineTest, jit_debug_x86) { EXPECT_EQ(0xffeb52a0U, unwinder.frames()[1].sp); EXPECT_EQ(0xec6061a8U, unwinder.frames()[2].pc); EXPECT_EQ(0xffeb5ce0U, unwinder.frames()[2].sp); - EXPECT_EQ(0xee75be81U, unwinder.frames()[3].pc); + EXPECT_EQ(0xee75be80U, unwinder.frames()[3].pc); EXPECT_EQ(0xffeb5d30U, unwinder.frames()[3].sp); EXPECT_EQ(0xf728e4d2U, unwinder.frames()[4].pc); EXPECT_EQ(0xffeb5d60U, unwinder.frames()[4].sp); @@ -435,7 +435,7 @@ TEST_F(UnwindOfflineTest, jit_debug_x86) { EXPECT_EQ(0xffeb5fb0U, unwinder.frames()[9].sp); EXPECT_EQ(0xf72945bdU, unwinder.frames()[10].pc); EXPECT_EQ(0xffeb6110U, unwinder.frames()[10].sp); - EXPECT_EQ(0xee75be04U, unwinder.frames()[11].pc); + EXPECT_EQ(0xee75be03U, unwinder.frames()[11].pc); EXPECT_EQ(0xffeb6160U, unwinder.frames()[11].sp); EXPECT_EQ(0xf728e4d2U, unwinder.frames()[12].pc); EXPECT_EQ(0xffeb6180U, unwinder.frames()[12].sp); @@ -451,7 +451,7 @@ TEST_F(UnwindOfflineTest, jit_debug_x86) { EXPECT_EQ(0xffeb63e0U, unwinder.frames()[17].sp); EXPECT_EQ(0xf72945bdU, unwinder.frames()[18].pc); EXPECT_EQ(0xffeb6530U, unwinder.frames()[18].sp); - EXPECT_EQ(0xee75bd3cU, unwinder.frames()[19].pc); + EXPECT_EQ(0xee75bd3bU, unwinder.frames()[19].pc); EXPECT_EQ(0xffeb6580U, unwinder.frames()[19].sp); EXPECT_EQ(0xf728e4d2U, unwinder.frames()[20].pc); EXPECT_EQ(0xffeb65b0U, unwinder.frames()[20].sp); @@ -467,7 +467,7 @@ TEST_F(UnwindOfflineTest, jit_debug_x86) { EXPECT_EQ(0xffeb6810U, unwinder.frames()[25].sp); EXPECT_EQ(0xf72945bdU, unwinder.frames()[26].pc); EXPECT_EQ(0xffeb6960U, unwinder.frames()[26].sp); - EXPECT_EQ(0xee75bbdcU, unwinder.frames()[27].pc); + EXPECT_EQ(0xee75bbdbU, unwinder.frames()[27].pc); EXPECT_EQ(0xffeb69b0U, unwinder.frames()[27].sp); EXPECT_EQ(0xf728e6a2U, unwinder.frames()[28].pc); EXPECT_EQ(0xffeb69f0U, unwinder.frames()[28].sp); @@ -483,7 +483,7 @@ TEST_F(UnwindOfflineTest, jit_debug_x86) { EXPECT_EQ(0xffeb6c50U, unwinder.frames()[33].sp); EXPECT_EQ(0xf72945bdU, unwinder.frames()[34].pc); EXPECT_EQ(0xffeb6dd0U, unwinder.frames()[34].sp); - EXPECT_EQ(0xee75b625U, unwinder.frames()[35].pc); + EXPECT_EQ(0xee75b624U, unwinder.frames()[35].pc); EXPECT_EQ(0xffeb6e20U, unwinder.frames()[35].sp); EXPECT_EQ(0xf728e4d2U, unwinder.frames()[36].pc); EXPECT_EQ(0xffeb6e50U, unwinder.frames()[36].sp); @@ -499,7 +499,7 @@ TEST_F(UnwindOfflineTest, jit_debug_x86) { EXPECT_EQ(0xffeb70a0U, unwinder.frames()[41].sp); EXPECT_EQ(0xf72945bdU, unwinder.frames()[42].pc); EXPECT_EQ(0xffeb71f0U, unwinder.frames()[42].sp); - EXPECT_EQ(0xee75aedcU, unwinder.frames()[43].pc); + EXPECT_EQ(0xee75aedbU, unwinder.frames()[43].pc); EXPECT_EQ(0xffeb7240U, unwinder.frames()[43].sp); EXPECT_EQ(0xf728e4d2U, unwinder.frames()[44].pc); EXPECT_EQ(0xffeb72a0U, unwinder.frames()[44].sp); @@ -515,7 +515,7 @@ TEST_F(UnwindOfflineTest, jit_debug_x86) { EXPECT_EQ(0xffeb74f0U, unwinder.frames()[49].sp); EXPECT_EQ(0xf72945bdU, unwinder.frames()[50].pc); EXPECT_EQ(0xffeb7680U, unwinder.frames()[50].sp); - EXPECT_EQ(0xee756c22U, unwinder.frames()[51].pc); + EXPECT_EQ(0xee756c21U, unwinder.frames()[51].pc); EXPECT_EQ(0xffeb76d0U, unwinder.frames()[51].sp); EXPECT_EQ(0xf728e6a2U, unwinder.frames()[52].pc); EXPECT_EQ(0xffeb76f0U, unwinder.frames()[52].sp); -- cgit v1.2.3 From 6eed2e505e8c939e01ea139e20bf7f587d564516 Mon Sep 17 00:00:00 2001 From: David Srbecky Date: Wed, 14 Mar 2018 21:30:25 +0000 Subject: Cache DWARF location rules for a given pc. Decoding the DWARF opcodes is expensive so make sure we cache it. This speeds unwinding in simpleperf by over a factor of 3x. Add unit tests for this new behavior. Bug: 77258731 Test: libbacktrace/libunwindstack unit tests on host and target. Test: Ran debuggerd -b on various processes on target. Change-Id: Ia516c0fa5d3e5f76746190bb4b6fdf49fd1c9388 (cherry picked from commit 3386ebade2d28fd3ef68c576bb0375bd226a1320) --- libunwindstack/DwarfCfa.cpp | 13 ++++- libunwindstack/DwarfSection.cpp | 30 ++++++---- libunwindstack/include/unwindstack/DwarfLocation.h | 10 +++- libunwindstack/include/unwindstack/DwarfSection.h | 2 + libunwindstack/tests/DwarfSectionImplTest.cpp | 3 +- libunwindstack/tests/DwarfSectionTest.cpp | 68 ++++++++++++++++++++++ 6 files changed, 111 insertions(+), 15 deletions(-) diff --git a/libunwindstack/DwarfCfa.cpp b/libunwindstack/DwarfCfa.cpp index aa8cd3a..6ecedce 100644 --- a/libunwindstack/DwarfCfa.cpp +++ b/libunwindstack/DwarfCfa.cpp @@ -50,7 +50,17 @@ bool DwarfCfa::GetLocationInfo(uint64_t pc, uint64_t start_offset, memory_->set_cur_offset(start_offset); uint64_t cfa_offset; cur_pc_ = fde_->pc_start; - while ((cfa_offset = memory_->cur_offset()) < end_offset && cur_pc_ <= pc) { + loc_regs->pc_start = cur_pc_; + while (true) { + if (cur_pc_ > pc) { + loc_regs->pc_end = cur_pc_; + return true; + } + if ((cfa_offset = memory_->cur_offset()) >= end_offset) { + loc_regs->pc_end = fde_->pc_end; + return true; + } + loc_regs->pc_start = cur_pc_; operands_.clear(); // Read the cfa information. uint8_t cfa_value; @@ -129,7 +139,6 @@ bool DwarfCfa::GetLocationInfo(uint64_t pc, uint64_t start_offset, } } } - return true; } template diff --git a/libunwindstack/DwarfSection.cpp b/libunwindstack/DwarfSection.cpp index 5586e72..65eec65 100644 --- a/libunwindstack/DwarfSection.cpp +++ b/libunwindstack/DwarfSection.cpp @@ -55,21 +55,29 @@ const DwarfFde* DwarfSection::GetFdeFromPc(uint64_t pc) { } bool DwarfSection::Step(uint64_t pc, Regs* regs, Memory* process_memory, bool* finished) { - last_error_.code = DWARF_ERROR_NONE; - const DwarfFde* fde = GetFdeFromPc(pc); - if (fde == nullptr || fde->cie == nullptr) { - last_error_.code = DWARF_ERROR_ILLEGAL_STATE; - return false; - } + // Lookup the pc in the cache. + auto it = loc_regs_.upper_bound(pc); + if (it == loc_regs_.end() || pc < it->second.pc_start) { + last_error_.code = DWARF_ERROR_NONE; + const DwarfFde* fde = GetFdeFromPc(pc); + if (fde == nullptr || fde->cie == nullptr) { + last_error_.code = DWARF_ERROR_ILLEGAL_STATE; + return false; + } - // Now get the location information for this pc. - dwarf_loc_regs_t loc_regs; - if (!GetCfaLocationInfo(pc, fde, &loc_regs)) { - return false; + // Now get the location information for this pc. + dwarf_loc_regs_t loc_regs; + if (!GetCfaLocationInfo(pc, fde, &loc_regs)) { + return false; + } + loc_regs.cie = fde->cie; + + // Store it in the cache. + it = loc_regs_.emplace(loc_regs.pc_end, std::move(loc_regs)).first; } // Now eval the actual registers. - return Eval(fde->cie, process_memory, loc_regs, regs, finished); + return Eval(it->second.cie, process_memory, it->second, regs, finished); } template diff --git a/libunwindstack/include/unwindstack/DwarfLocation.h b/libunwindstack/include/unwindstack/DwarfLocation.h index 0881182..3d50ccf 100644 --- a/libunwindstack/include/unwindstack/DwarfLocation.h +++ b/libunwindstack/include/unwindstack/DwarfLocation.h @@ -23,6 +23,8 @@ namespace unwindstack { +struct DwarfCie; + enum DwarfLocationEnum : uint8_t { DWARF_LOCATION_INVALID = 0, DWARF_LOCATION_UNDEFINED, @@ -38,7 +40,13 @@ struct DwarfLocation { uint64_t values[2]; }; -typedef std::unordered_map dwarf_loc_regs_t; +struct DwarfLocations : public std::unordered_map { + const DwarfCie* cie; + // The range of PCs where the locations are valid (end is exclusive). + uint64_t pc_start = 0; + uint64_t pc_end = 0; +}; +typedef DwarfLocations dwarf_loc_regs_t; } // namespace unwindstack diff --git a/libunwindstack/include/unwindstack/DwarfSection.h b/libunwindstack/include/unwindstack/DwarfSection.h index da91fd0..209c54a 100644 --- a/libunwindstack/include/unwindstack/DwarfSection.h +++ b/libunwindstack/include/unwindstack/DwarfSection.h @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -112,6 +113,7 @@ class DwarfSection { std::unordered_map fde_entries_; std::unordered_map cie_entries_; std::unordered_map cie_loc_regs_; + std::map loc_regs_; // Single row indexed by pc_end. }; template diff --git a/libunwindstack/tests/DwarfSectionImplTest.cpp b/libunwindstack/tests/DwarfSectionImplTest.cpp index c85764c..c340291 100644 --- a/libunwindstack/tests/DwarfSectionImplTest.cpp +++ b/libunwindstack/tests/DwarfSectionImplTest.cpp @@ -853,7 +853,8 @@ TYPED_TEST_P(DwarfSectionImplTest, GetCfaLocationInfo_cie_cached) { fde.cfa_instructions_offset = 0x6000; fde.cfa_instructions_end = 0x6002; - dwarf_loc_regs_t cie_loc_regs{{6, {DWARF_LOCATION_REGISTER, {4, 0}}}}; + dwarf_loc_regs_t cie_loc_regs; + cie_loc_regs[6] = DwarfLocation{DWARF_LOCATION_REGISTER, {4, 0}}; this->section_->TestSetCachedCieLocRegs(0x8000, cie_loc_regs); this->memory_.SetMemory(0x6000, std::vector{0x09, 0x04, 0x03}); diff --git a/libunwindstack/tests/DwarfSectionTest.cpp b/libunwindstack/tests/DwarfSectionTest.cpp index 3fcd2b6..071d2df 100644 --- a/libunwindstack/tests/DwarfSectionTest.cpp +++ b/libunwindstack/tests/DwarfSectionTest.cpp @@ -165,4 +165,72 @@ TEST_F(DwarfSectionTest, Step_pass) { ASSERT_TRUE(mock_section.Step(0x1000, nullptr, &process, &finished)); } +static bool MockGetCfaLocationInfo(::testing::Unused, const DwarfFde* fde, + dwarf_loc_regs_t* loc_regs) { + loc_regs->pc_start = fde->pc_start; + loc_regs->pc_end = fde->pc_end; + return true; +} + +TEST_F(DwarfSectionTest, Step_cache) { + MockDwarfSection mock_section(&memory_); + + DwarfCie cie{}; + DwarfFde fde{}; + fde.pc_start = 0x500; + fde.pc_end = 0x2000; + fde.cie = &cie; + + EXPECT_CALL(mock_section, GetFdeOffsetFromPc(0x1000, ::testing::_)) + .WillOnce(::testing::Return(true)); + EXPECT_CALL(mock_section, GetFdeFromOffset(::testing::_)).WillOnce(::testing::Return(&fde)); + + EXPECT_CALL(mock_section, GetCfaLocationInfo(0x1000, &fde, ::testing::_)) + .WillOnce(::testing::Invoke(MockGetCfaLocationInfo)); + + MemoryFake process; + EXPECT_CALL(mock_section, Eval(&cie, &process, ::testing::_, nullptr, ::testing::_)) + .WillRepeatedly(::testing::Return(true)); + + bool finished; + ASSERT_TRUE(mock_section.Step(0x1000, nullptr, &process, &finished)); + ASSERT_TRUE(mock_section.Step(0x1000, nullptr, &process, &finished)); + ASSERT_TRUE(mock_section.Step(0x1500, nullptr, &process, &finished)); +} + +TEST_F(DwarfSectionTest, Step_cache_not_in_pc) { + MockDwarfSection mock_section(&memory_); + + DwarfCie cie{}; + DwarfFde fde0{}; + fde0.pc_start = 0x1000; + fde0.pc_end = 0x2000; + fde0.cie = &cie; + EXPECT_CALL(mock_section, GetFdeOffsetFromPc(0x1000, ::testing::_)) + .WillOnce(::testing::Return(true)); + EXPECT_CALL(mock_section, GetFdeFromOffset(::testing::_)).WillOnce(::testing::Return(&fde0)); + EXPECT_CALL(mock_section, GetCfaLocationInfo(0x1000, &fde0, ::testing::_)) + .WillOnce(::testing::Invoke(MockGetCfaLocationInfo)); + + MemoryFake process; + EXPECT_CALL(mock_section, Eval(&cie, &process, ::testing::_, nullptr, ::testing::_)) + .WillRepeatedly(::testing::Return(true)); + + bool finished; + ASSERT_TRUE(mock_section.Step(0x1000, nullptr, &process, &finished)); + + DwarfFde fde1{}; + fde1.pc_start = 0x500; + fde1.pc_end = 0x800; + fde1.cie = &cie; + EXPECT_CALL(mock_section, GetFdeOffsetFromPc(0x600, ::testing::_)) + .WillOnce(::testing::Return(true)); + EXPECT_CALL(mock_section, GetFdeFromOffset(::testing::_)).WillOnce(::testing::Return(&fde1)); + EXPECT_CALL(mock_section, GetCfaLocationInfo(0x600, &fde1, ::testing::_)) + .WillOnce(::testing::Invoke(MockGetCfaLocationInfo)); + + ASSERT_TRUE(mock_section.Step(0x600, nullptr, &process, &finished)); + ASSERT_TRUE(mock_section.Step(0x700, nullptr, &process, &finished)); +} + } // namespace unwindstack -- cgit v1.2.3