From fc1b9d3203ecb505154afdb70702864605c99918 Mon Sep 17 00:00:00 2001 From: Ivan Penkov Date: Thu, 17 Feb 2022 03:18:03 +0000 Subject: [PATCH] Populating is_multiple in google_breakpad::StackFrame from symbol files. This is needed in order to properly detect and highlight frames that correspond to multiple functions, for example as the result of identical code folding by the linker. Bug: google-breakpad:751 Change-Id: I2ee7c147fcff6493c2454383ad5422b38269759a Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3471034 Reviewed-by: Joshua Peraza --- src/google_breakpad/processor/stack_frame.h | 9 +++++++- src/processor/basic_source_line_resolver.cc | 2 ++ .../basic_source_line_resolver_unittest.cc | 22 ++++++++++++------- src/processor/fast_source_line_resolver.cc | 2 ++ .../fast_source_line_resolver_unittest.cc | 8 ++++++- src/processor/testdata/module1.out | 4 ++-- 6 files changed, 35 insertions(+), 12 deletions(-) diff --git a/src/google_breakpad/processor/stack_frame.h b/src/google_breakpad/processor/stack_frame.h index 5f3932b6..18e95fbc 100644 --- a/src/google_breakpad/processor/stack_frame.h +++ b/src/google_breakpad/processor/stack_frame.h @@ -63,7 +63,8 @@ struct StackFrame { source_file_name(), source_line(0), source_line_base(), - trust(FRAME_TRUST_NONE){} + trust(FRAME_TRUST_NONE), + is_multiple(false) {} virtual ~StackFrame() {} // Return a string describing how this stack frame was found @@ -140,6 +141,12 @@ struct StackFrame { // Amount of trust the stack walker has in the instruction pointer // of this frame. FrameTrust trust; + + // True if the frame corresponds to multiple functions, for example as the + // result of identical code folding by the linker. In that case the function + // name, filename, etc. information above represents the state of an arbitrary + // one of these functions. + bool is_multiple; }; } // namespace google_breakpad diff --git a/src/processor/basic_source_line_resolver.cc b/src/processor/basic_source_line_resolver.cc index dccbd74b..e525d4f9 100644 --- a/src/processor/basic_source_line_resolver.cc +++ b/src/processor/basic_source_line_resolver.cc @@ -319,6 +319,7 @@ void BasicSourceLineResolver::Module::LookupAddress( address >= function_base && address - function_base < function_size) { frame->function_name = func->name; frame->function_base = frame->module->base_address() + function_base; + frame->is_multiple = func->is_multiple; linked_ptr line; MemAddr line_base; @@ -341,6 +342,7 @@ void BasicSourceLineResolver::Module::LookupAddress( (!func.get() || public_address > function_base)) { frame->function_name = public_symbol->name; frame->function_base = frame->module->base_address() + public_address; + frame->is_multiple = public_symbol->is_multiple; } } diff --git a/src/processor/basic_source_line_resolver_unittest.cc b/src/processor/basic_source_line_resolver_unittest.cc index e608a548..a9f1a886 100644 --- a/src/processor/basic_source_line_resolver_unittest.cc +++ b/src/processor/basic_source_line_resolver_unittest.cc @@ -52,7 +52,6 @@ using google_breakpad::CodeModule; using google_breakpad::MemoryRegion; using google_breakpad::StackFrame; using google_breakpad::WindowsFrameInfo; -using google_breakpad::linked_ptr; using google_breakpad::scoped_ptr; using google_breakpad::SymbolParseHelper; @@ -93,12 +92,12 @@ class MockMemoryRegion: public MemoryRegion { } bool GetMemoryAtAddress(uint64_t address, uint32_t* value) const { switch (address) { - case 0x10008: *value = 0x98ecadc3; break; // saved %ebx - case 0x1000c: *value = 0x878f7524; break; // saved %esi - case 0x10010: *value = 0x6312f9a5; break; // saved %edi - case 0x10014: *value = 0x10038; break; // caller's %ebp - case 0x10018: *value = 0xf6438648; break; // return address - default: *value = 0xdeadbeef; break; // junk + case 0x10008: *value = 0x98ecadc3; break; // saved %ebx + case 0x1000c: *value = 0x878f7524; break; // saved %esi + case 0x10010: *value = 0x6312f9a5; break; // saved %edi + case 0x10014: *value = 0x10038; break; // caller's %ebp + case 0x10018: *value = 0xf6438648; break; // return address + default: *value = 0xdeadbeef; break; // junk } return true; } @@ -164,7 +163,7 @@ static void ClearSourceLineInfo(StackFrame* frame) { } class TestBasicSourceLineResolver : public ::testing::Test { -public: + public: void SetUp() { testdata_dir = string(getenv("srcdir") ? getenv("srcdir") : ".") + "/src/processor/testdata"; @@ -196,6 +195,7 @@ TEST_F(TestBasicSourceLineResolver, TestLoadAndResolve) ASSERT_TRUE(frame.source_file_name.empty()); ASSERT_EQ(frame.source_line, 0); ASSERT_EQ(frame.source_line_base, 0U); + EXPECT_EQ(frame.is_multiple, false); frame.module = &module1; resolver.FillSourceLineInfo(&frame, nullptr); @@ -206,6 +206,7 @@ TEST_F(TestBasicSourceLineResolver, TestLoadAndResolve) ASSERT_EQ(frame.source_file_name, "file1_1.cc"); ASSERT_EQ(frame.source_line, 44); ASSERT_EQ(frame.source_line_base, 0x1000U); + EXPECT_EQ(frame.is_multiple, true); windows_frame_info.reset(resolver.FindWindowsFrameInfo(&frame)); ASSERT_TRUE(windows_frame_info.get()); ASSERT_EQ(windows_frame_info->type_, WindowsFrameInfo::STACK_INFO_FRAME_DATA); @@ -344,6 +345,7 @@ TEST_F(TestBasicSourceLineResolver, TestLoadAndResolve) frame.module = &module1; resolver.FillSourceLineInfo(&frame, nullptr); ASSERT_EQ(frame.function_name, string("PublicSymbol")); + EXPECT_EQ(frame.is_multiple, true); frame.instruction = 0x4000; frame.module = &module1; @@ -360,6 +362,7 @@ TEST_F(TestBasicSourceLineResolver, TestLoadAndResolve) ASSERT_EQ(frame.source_file_name, "file2_2.cc"); ASSERT_EQ(frame.source_line, 21); ASSERT_EQ(frame.source_line_base, 0x2180U); + EXPECT_EQ(frame.is_multiple, false); windows_frame_info.reset(resolver.FindWindowsFrameInfo(&frame)); ASSERT_TRUE(windows_frame_info.get()); ASSERT_EQ(windows_frame_info->type_, WindowsFrameInfo::STACK_INFO_FRAME_DATA); @@ -368,6 +371,7 @@ TEST_F(TestBasicSourceLineResolver, TestLoadAndResolve) frame.instruction = 0x216f; resolver.FillSourceLineInfo(&frame, nullptr); ASSERT_EQ(frame.function_name, "Public2_1"); + EXPECT_EQ(frame.is_multiple, false); ClearSourceLineInfo(&frame); frame.instruction = 0x219f; @@ -431,6 +435,7 @@ TEST_F(TestBasicSourceLineResolver, TestLoadAndResolveOldInlines) { ASSERT_EQ(frame.source_file_name, "linux_inline.cpp"); ASSERT_EQ(frame.source_line, 42); ASSERT_EQ(frame.source_line_base, 0x161b6U); + EXPECT_EQ(frame.is_multiple, false); ASSERT_EQ(inlined_frames.size(), 3UL); @@ -475,6 +480,7 @@ TEST_F(TestBasicSourceLineResolver, TestLoadAndResolveNewInlines) { ASSERT_EQ(frame.source_file_name, "a.cpp"); ASSERT_EQ(frame.source_line, 42); ASSERT_EQ(frame.source_line_base, 0x161b6U); + EXPECT_EQ(frame.is_multiple, false); ASSERT_EQ(inlined_frames.size(), 3UL); diff --git a/src/processor/fast_source_line_resolver.cc b/src/processor/fast_source_line_resolver.cc index 81a6b7ac..1fbe06fa 100644 --- a/src/processor/fast_source_line_resolver.cc +++ b/src/processor/fast_source_line_resolver.cc @@ -88,6 +88,7 @@ void FastSourceLineResolver::Module::LookupAddress( func->CopyFrom(func_ptr); frame->function_name = func->name; frame->function_base = frame->module->base_address() + function_base; + frame->is_multiple = func->is_multiple; scoped_ptr line(new Line); const Line* line_ptr = 0; @@ -112,6 +113,7 @@ void FastSourceLineResolver::Module::LookupAddress( public_symbol->CopyFrom(public_symbol_ptr); frame->function_name = public_symbol->name; frame->function_base = frame->module->base_address() + public_address; + frame->is_multiple = public_symbol->is_multiple; } } diff --git a/src/processor/fast_source_line_resolver_unittest.cc b/src/processor/fast_source_line_resolver_unittest.cc index a8e22408..6ef443ca 100644 --- a/src/processor/fast_source_line_resolver_unittest.cc +++ b/src/processor/fast_source_line_resolver_unittest.cc @@ -64,7 +64,6 @@ using google_breakpad::CodeModule; using google_breakpad::MemoryRegion; using google_breakpad::StackFrame; using google_breakpad::WindowsFrameInfo; -using google_breakpad::linked_ptr; using google_breakpad::scoped_ptr; class TestCodeModule : public CodeModule { @@ -224,6 +223,7 @@ TEST_F(TestFastSourceLineResolver, TestLoadAndResolve) { ASSERT_TRUE(frame.source_file_name.empty()); ASSERT_EQ(frame.source_line, 0); ASSERT_EQ(frame.source_line_base, 0U); + ASSERT_EQ(frame.is_multiple, false); frame.module = &module1; fast_resolver.FillSourceLineInfo(&frame, nullptr); @@ -234,6 +234,7 @@ TEST_F(TestFastSourceLineResolver, TestLoadAndResolve) { ASSERT_EQ(frame.source_file_name, "file1_1.cc"); ASSERT_EQ(frame.source_line, 44); ASSERT_EQ(frame.source_line_base, 0x1000U); + ASSERT_EQ(frame.is_multiple, true); windows_frame_info.reset(fast_resolver.FindWindowsFrameInfo(&frame)); ASSERT_TRUE(windows_frame_info.get()); ASSERT_FALSE(windows_frame_info->allocates_base_pointer); @@ -371,6 +372,7 @@ TEST_F(TestFastSourceLineResolver, TestLoadAndResolve) { frame.module = &module1; fast_resolver.FillSourceLineInfo(&frame, nullptr); ASSERT_EQ(frame.function_name, string("PublicSymbol")); + EXPECT_EQ(frame.is_multiple, true); frame.instruction = 0x4000; frame.module = &module1; @@ -387,6 +389,7 @@ TEST_F(TestFastSourceLineResolver, TestLoadAndResolve) { ASSERT_EQ(frame.source_file_name, "file2_2.cc"); ASSERT_EQ(frame.source_line, 21); ASSERT_EQ(frame.source_line_base, 0x2180U); + ASSERT_EQ(frame.is_multiple, false); windows_frame_info.reset(fast_resolver.FindWindowsFrameInfo(&frame)); ASSERT_TRUE(windows_frame_info.get()); ASSERT_EQ(windows_frame_info->type_, WindowsFrameInfo::STACK_INFO_FRAME_DATA); @@ -395,6 +398,7 @@ TEST_F(TestFastSourceLineResolver, TestLoadAndResolve) { frame.instruction = 0x216f; fast_resolver.FillSourceLineInfo(&frame, nullptr); ASSERT_EQ(frame.function_name, "Public2_1"); + EXPECT_EQ(frame.is_multiple, false); ClearSourceLineInfo(&frame); frame.instruction = 0x219f; @@ -433,6 +437,7 @@ TEST_F(TestFastSourceLineResolver, TestLoadAndResolveOldInlines) { ASSERT_EQ(frame.source_file_name, "linux_inline.cpp"); ASSERT_EQ(frame.source_line, 42); ASSERT_EQ(frame.source_line_base, 0x161b6U); + ASSERT_EQ(frame.is_multiple, false); ASSERT_EQ(inlined_frames.size(), 3UL); @@ -484,6 +489,7 @@ TEST_F(TestFastSourceLineResolver, TestLoadAndResolveNewInlines) { ASSERT_EQ(frame.source_file_name, "a.cpp"); ASSERT_EQ(frame.source_line, 42); ASSERT_EQ(frame.source_line_base, 0x161b6U); + ASSERT_EQ(frame.is_multiple, false); ASSERT_EQ(inlined_frames.size(), 3UL); diff --git a/src/processor/testdata/module1.out b/src/processor/testdata/module1.out index cd6e18d1..6774e4f1 100644 --- a/src/processor/testdata/module1.out +++ b/src/processor/testdata/module1.out @@ -3,7 +3,7 @@ INFO CODE_ID FFFFFFFF module1.exe FILE 1 file1_1.cc FILE 2 file1_2.cc FILE 3 file1_3.cc -FUNC 1000 c 0 Function1_1 +FUNC m 1000 c 0 Function1_1 1000 4 44 1 1004 4 45 1 1008 4 46 1 @@ -14,7 +14,7 @@ FUNC 1200 100 8 Function1_3 FUNC 1300 100 c Function1_4 FUNC 2000 0 0 Test_Zero_Size_Function_Is_Ignored 2000 4 88 2 -PUBLIC 2800 0 PublicSymbol +PUBLIC m 2800 0 PublicSymbol FUNC 3000 7000 42 LargeFunction 3000 7000 4098359 3 STACK WIN 4 1000 c 1 0 0 0 0 0 1 $eip 4 + ^ = $esp $ebp 8 + = $ebp $ebp ^ =