From fda7edd13ec5970e43e9677dae066f52b277a470 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Tue, 31 Oct 2017 16:10:42 -0700 Subject: [PATCH] Move sp/pc not changing check into Unwinder. Remove this check from the DwarfSection class. Rather than have every step function make the check, doing it at the top level avoids having every function do the same check. Bug: 68167269 Test: New unit tests, ran debuggerd -b on processes. Change-Id: I23b7c799faaf26c93c1b72848df18c78de6c42fb --- libunwindstack/DwarfSection.cpp | 5 +- libunwindstack/Unwinder.cpp | 10 ++- libunwindstack/tests/DwarfSectionImplTest.cpp | 22 +------ libunwindstack/tests/UnwinderTest.cpp | 66 +++++++++++++++++++ 4 files changed, 80 insertions(+), 23 deletions(-) diff --git a/libunwindstack/DwarfSection.cpp b/libunwindstack/DwarfSection.cpp index b8164c577..22921684a 100644 --- a/libunwindstack/DwarfSection.cpp +++ b/libunwindstack/DwarfSection.cpp @@ -107,7 +107,6 @@ bool DwarfSectionImpl::Eval(const DwarfCie* cie, Memory* regular_me return false; } - AddressType prev_pc = regs->pc(); AddressType prev_cfa = regs->sp(); AddressType cfa; @@ -233,8 +232,8 @@ bool DwarfSectionImpl::Eval(const DwarfCie* cie, Memory* regular_me *finished = (cur_regs->pc() == 0) ? true : false; cur_regs->set_sp(cfa); - // Return false if the unwind is not finished or the cfa and pc didn't change. - return *finished || prev_cfa != cfa || prev_pc != cur_regs->pc(); + + return true; } template diff --git a/libunwindstack/Unwinder.cpp b/libunwindstack/Unwinder.cpp index 2190711ec..2e46a115e 100644 --- a/libunwindstack/Unwinder.cpp +++ b/libunwindstack/Unwinder.cpp @@ -87,8 +87,10 @@ void Unwinder::Unwind(const std::vector* initial_map_names_to_skip, bool return_address_attempt = false; bool adjust_pc = false; for (; frames_.size() < max_frames_;) { - MapInfo* map_info = maps_->Find(regs_->pc()); + uint64_t cur_pc = regs_->pc(); + uint64_t cur_sp = regs_->sp(); + MapInfo* map_info = maps_->Find(regs_->pc()); uint64_t rel_pc; Elf* elf; if (map_info == nullptr) { @@ -138,6 +140,7 @@ void Unwinder::Unwind(const std::vector* initial_map_names_to_skip, } } } + if (!stepped) { if (return_address_attempt) { // Remove the speculative frame. @@ -157,6 +160,11 @@ void Unwinder::Unwind(const std::vector* initial_map_names_to_skip, } else { return_address_attempt = false; } + + // If the pc and sp didn't change, then consider everything stopped. + if (cur_pc == regs_->pc() && cur_sp == regs_->sp()) { + break; + } } } diff --git a/libunwindstack/tests/DwarfSectionImplTest.cpp b/libunwindstack/tests/DwarfSectionImplTest.cpp index 7e85bbbf8..5b9f3ee19 100644 --- a/libunwindstack/tests/DwarfSectionImplTest.cpp +++ b/libunwindstack/tests/DwarfSectionImplTest.cpp @@ -431,22 +431,6 @@ TYPED_TEST_P(DwarfSectionImplTest, Eval_reg_val_expr) { EXPECT_EQ(0x80000000U, regs.pc()); } -TYPED_TEST_P(DwarfSectionImplTest, Eval_same_cfa_same_pc) { - DwarfCie cie{.version = 3, .return_address_register = 5}; - RegsImplFake regs(10, 9); - dwarf_loc_regs_t loc_regs; - - regs.set_pc(0x100); - regs.set_sp(0x2000); - regs[5] = 0x100; - regs[8] = 0x2000; - loc_regs[CFA_REG] = DwarfLocation{DWARF_LOCATION_REGISTER, {8, 0}}; - bool finished; - ASSERT_FALSE(this->section_->Eval(&cie, &this->memory_, loc_regs, ®s, &finished)); - EXPECT_EQ(0x2000U, regs.sp()); - EXPECT_EQ(0x100U, regs.pc()); -} - TYPED_TEST_P(DwarfSectionImplTest, GetCie_fail_should_not_cache) { ASSERT_TRUE(this->section_->GetCie(0x4000) == nullptr); EXPECT_EQ(DWARF_ERROR_MEMORY_INVALID, this->section_->last_error()); @@ -872,9 +856,9 @@ REGISTER_TYPED_TEST_CASE_P( Eval_cfa_bad, Eval_cfa_register_prev, Eval_cfa_register_from_value, Eval_double_indirection, Eval_invalid_register, Eval_different_reg_locations, Eval_return_address_undefined, Eval_pc_zero, Eval_return_address, Eval_ignore_large_reg_loc, Eval_reg_expr, Eval_reg_val_expr, - Eval_same_cfa_same_pc, GetCie_fail_should_not_cache, GetCie_32_version_check, - GetCie_negative_data_alignment_factor, GetCie_64_no_augment, GetCie_augment, GetCie_version_3, - GetCie_version_4, GetFdeFromOffset_fail_should_not_cache, GetFdeFromOffset_32_no_augment, + GetCie_fail_should_not_cache, GetCie_32_version_check, GetCie_negative_data_alignment_factor, + GetCie_64_no_augment, GetCie_augment, GetCie_version_3, GetCie_version_4, + GetFdeFromOffset_fail_should_not_cache, GetFdeFromOffset_32_no_augment, GetFdeFromOffset_32_no_augment_non_zero_segment_size, GetFdeFromOffset_32_augment, GetFdeFromOffset_64_no_augment, GetFdeFromOffset_cached, GetCfaLocationInfo_cie_not_cached, GetCfaLocationInfo_cie_cached, Log); diff --git a/libunwindstack/tests/UnwinderTest.cpp b/libunwindstack/tests/UnwinderTest.cpp index 8a90baec0..869d1189e 100644 --- a/libunwindstack/tests/UnwinderTest.cpp +++ b/libunwindstack/tests/UnwinderTest.cpp @@ -127,6 +127,7 @@ class UnwinderTest : public ::testing::Test { void SetUp() override { ElfInterfaceFake::FakeClear(); regs_.FakeSetMachineType(EM_ARM); + regs_.FakeSetReturnAddressValid(false); } static MapsFake maps_; @@ -610,6 +611,71 @@ TEST_F(UnwinderTest, map_ignore_suffixes) { EXPECT_EQ(PROT_READ | PROT_WRITE, frame->map_flags); } +// Verify that an unwind stops when the sp and pc don't change. +TEST_F(UnwinderTest, sp_pc_do_not_change) { + ElfInterfaceFake::FakePushFunctionData(FunctionData("Frame0", 0)); + ElfInterfaceFake::FakePushFunctionData(FunctionData("Frame1", 1)); + ElfInterfaceFake::FakePushFunctionData(FunctionData("Frame2", 2)); + ElfInterfaceFake::FakePushFunctionData(FunctionData("Frame3", 3)); + ElfInterfaceFake::FakePushFunctionData(FunctionData("Frame4", 4)); + + regs_.FakeSetPc(0x1000); + regs_.FakeSetSp(0x10000); + ElfInterfaceFake::FakePushStepData(StepData(0x33402, 0x10010, false)); + ElfInterfaceFake::FakePushStepData(StepData(0x33502, 0x10020, false)); + ElfInterfaceFake::FakePushStepData(StepData(0x33502, 0x10020, false)); + ElfInterfaceFake::FakePushStepData(StepData(0x33502, 0x10020, false)); + ElfInterfaceFake::FakePushStepData(StepData(0x33502, 0x10020, false)); + ElfInterfaceFake::FakePushStepData(StepData(0, 0, true)); + + Unwinder unwinder(64, &maps_, ®s_, process_memory_); + unwinder.Unwind(); + + ASSERT_EQ(3U, unwinder.NumFrames()); + + auto* frame = &unwinder.frames()[0]; + EXPECT_EQ(0U, frame->num); + EXPECT_EQ(0U, frame->rel_pc); + EXPECT_EQ(0x1000U, frame->pc); + EXPECT_EQ(0x10000U, frame->sp); + EXPECT_EQ("Frame0", frame->function_name); + EXPECT_EQ(0U, frame->function_offset); + EXPECT_EQ("/system/fake/libc.so", frame->map_name); + EXPECT_EQ(0U, frame->map_offset); + EXPECT_EQ(0x1000U, frame->map_start); + EXPECT_EQ(0x8000U, frame->map_end); + EXPECT_EQ(0U, frame->map_load_bias); + EXPECT_EQ(PROT_READ | PROT_WRITE, frame->map_flags); + + frame = &unwinder.frames()[1]; + EXPECT_EQ(1U, frame->num); + EXPECT_EQ(0x400U, frame->rel_pc); + EXPECT_EQ(0x33400U, frame->pc); + EXPECT_EQ(0x10010U, frame->sp); + EXPECT_EQ("Frame1", frame->function_name); + EXPECT_EQ(1U, frame->function_offset); + EXPECT_EQ("/fake/compressed.so", frame->map_name); + EXPECT_EQ(0U, frame->map_offset); + EXPECT_EQ(0x33000U, frame->map_start); + EXPECT_EQ(0x34000U, frame->map_end); + EXPECT_EQ(0U, frame->map_load_bias); + EXPECT_EQ(PROT_READ | PROT_WRITE, frame->map_flags); + + frame = &unwinder.frames()[2]; + EXPECT_EQ(2U, frame->num); + EXPECT_EQ(0x500U, frame->rel_pc); + EXPECT_EQ(0x33500U, frame->pc); + EXPECT_EQ(0x10020U, frame->sp); + EXPECT_EQ("Frame2", frame->function_name); + EXPECT_EQ(2U, frame->function_offset); + EXPECT_EQ("/fake/compressed.so", frame->map_name); + EXPECT_EQ(0U, frame->map_offset); + EXPECT_EQ(0x33000U, frame->map_start); + EXPECT_EQ(0x34000U, frame->map_end); + EXPECT_EQ(0U, frame->map_load_bias); + EXPECT_EQ(PROT_READ | PROT_WRITE, frame->map_flags); +} + // Verify format frame code. TEST_F(UnwinderTest, format_frame_static) { FrameData frame;