https://github.com/Michael137 updated https://github.com/llvm/llvm-project/pull/74772
>From 2352f67789c04f86c8a0f7ad8940d782288750b8 Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Thu, 7 Dec 2023 21:35:41 +0000 Subject: [PATCH 1/2] [lldb][Symbol] Make sure we decrement PC before checking location list In optimized code we can end up with return address being the next instruction in a different block. If we are dealing with location lists, we want to decrement the PC value so it's within the calling block range that we're checking against the loclists. This was fixed in https://reviews.llvm.org/D124597 (`6e56c4961a106b8fde69ffcf9f803fe0890722fa`), by introducing a `GetFrameCodeAddressForSymbolication`. This fixed `frame variable`, but `expr` calls into `Variable::LocationIsValidForFrame`, where this new API wasn't used yet. So in the associated test-case, running `frame var this` works, but `expr this` doesn't. With `dwim-print` this makes the situation more surprising for the user because `p this`, `v this` and `v *this` work, but `p *this` doesn't because it falls back onto `expr` (due to the dereference). This patch makes sure we lookup loclists using the correct PC by using this new `GetFrameCodeAddressForSymbolication` API. --- lldb/source/Symbol/Variable.cpp | 3 +- .../location-list-lookup-cpp-member/Makefile | 3 ++ .../TestCppMemberLocationListLookup.py | 29 +++++++++++++++++++ .../location-list-lookup-cpp-member/main.cpp | 23 +++++++++++++++ 4 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 lldb/test/API/functionalities/location-list-lookup-cpp-member/Makefile create mode 100644 lldb/test/API/functionalities/location-list-lookup-cpp-member/TestCppMemberLocationListLookup.py create mode 100644 lldb/test/API/functionalities/location-list-lookup-cpp-member/main.cpp diff --git a/lldb/source/Symbol/Variable.cpp b/lldb/source/Symbol/Variable.cpp index 85ceadd20c611e..db740cb7cb6e41 100644 --- a/lldb/source/Symbol/Variable.cpp +++ b/lldb/source/Symbol/Variable.cpp @@ -227,7 +227,8 @@ bool Variable::LocationIsValidForFrame(StackFrame *frame) { // contains the current address when converted to a load address return m_location_list.ContainsAddress( loclist_base_load_addr, - frame->GetFrameCodeAddress().GetLoadAddress(target_sp.get())); + frame->GetFrameCodeAddressForSymbolication().GetLoadAddress( + target_sp.get())); } } return false; diff --git a/lldb/test/API/functionalities/location-list-lookup-cpp-member/Makefile b/lldb/test/API/functionalities/location-list-lookup-cpp-member/Makefile new file mode 100644 index 00000000000000..8e453681d7b390 --- /dev/null +++ b/lldb/test/API/functionalities/location-list-lookup-cpp-member/Makefile @@ -0,0 +1,3 @@ +CXX_SOURCES := main.cpp +CFLAGS_EXTRAS := -O1 +include Makefile.rules diff --git a/lldb/test/API/functionalities/location-list-lookup-cpp-member/TestCppMemberLocationListLookup.py b/lldb/test/API/functionalities/location-list-lookup-cpp-member/TestCppMemberLocationListLookup.py new file mode 100644 index 00000000000000..846386ceffc6ac --- /dev/null +++ b/lldb/test/API/functionalities/location-list-lookup-cpp-member/TestCppMemberLocationListLookup.py @@ -0,0 +1,29 @@ +"""Test that lldb picks the correct DWARF location list entry with a return-pc out of bounds.""" + +import lldb +from lldbsuite.test.lldbtest import * +from lldbsuite.test import lldbutil + + +class CppMemberLocationListLookupTestCase(TestBase): + def test(self): + self.build() + + exe = self.getBuildArtifact("a.out") + target = self.dbg.CreateTarget(exe) + self.assertTrue(target, VALID_TARGET) + self.dbg.SetAsync(False) + + li = lldb.SBLaunchInfo(["a.out"]) + error = lldb.SBError() + process = target.Launch(li, error) + self.assertTrue(process.IsValid()) + self.assertTrue(process.is_stopped) + + # Find `bar` on the stack, then + # find `this` local variable, then + # check that we can read out the pointer value + for f in process.GetSelectedThread().frames: + if f.GetDisplayFunctionName().startswith("Foo::bar"): + process.GetSelectedThread().SetSelectedFrame(f.idx) + self.expect_expr("this", result_type="Foo *") diff --git a/lldb/test/API/functionalities/location-list-lookup-cpp-member/main.cpp b/lldb/test/API/functionalities/location-list-lookup-cpp-member/main.cpp new file mode 100644 index 00000000000000..96817f141b82cb --- /dev/null +++ b/lldb/test/API/functionalities/location-list-lookup-cpp-member/main.cpp @@ -0,0 +1,23 @@ +#include <cstdio> +#include <cstdlib> + +void func(int in); + +struct Foo { + int x; + [[clang::noinline]] void bar(Foo *f); +}; + +int main(int argc, char **argv) { + Foo f{.x = 5}; + std::printf("%p\n", &f.x); + f.bar(&f); + return f.x; +} + +void Foo::bar(Foo *f) { + std::printf("%p %p\n", f, this); + std::abort(); /// 'this' should be still accessible +} + +void func(int in) { printf("%d\n", in); } >From 3a56e376861720ef23b4dcc57e99553945777269 Mon Sep 17 00:00:00 2001 From: Michael Buch <michaelbuc...@gmail.com> Date: Thu, 7 Dec 2023 22:54:19 +0000 Subject: [PATCH 2/2] fixup! combine location-list-lookup tests --- .../location-list-lookup-cpp-member/Makefile | 3 -- .../TestCppMemberLocationListLookup.py | 29 ------------------- .../location-list-lookup/Makefile | 2 +- .../TestLocationListLookup.py | 24 +++++++-------- .../location-list-lookup/main.c | 23 --------------- .../main.cpp | 8 ++--- 6 files changed, 15 insertions(+), 74 deletions(-) delete mode 100644 lldb/test/API/functionalities/location-list-lookup-cpp-member/Makefile delete mode 100644 lldb/test/API/functionalities/location-list-lookup-cpp-member/TestCppMemberLocationListLookup.py delete mode 100644 lldb/test/API/functionalities/location-list-lookup/main.c rename lldb/test/API/functionalities/{location-list-lookup-cpp-member => location-list-lookup}/main.cpp (68%) diff --git a/lldb/test/API/functionalities/location-list-lookup-cpp-member/Makefile b/lldb/test/API/functionalities/location-list-lookup-cpp-member/Makefile deleted file mode 100644 index 8e453681d7b390..00000000000000 --- a/lldb/test/API/functionalities/location-list-lookup-cpp-member/Makefile +++ /dev/null @@ -1,3 +0,0 @@ -CXX_SOURCES := main.cpp -CFLAGS_EXTRAS := -O1 -include Makefile.rules diff --git a/lldb/test/API/functionalities/location-list-lookup-cpp-member/TestCppMemberLocationListLookup.py b/lldb/test/API/functionalities/location-list-lookup-cpp-member/TestCppMemberLocationListLookup.py deleted file mode 100644 index 846386ceffc6ac..00000000000000 --- a/lldb/test/API/functionalities/location-list-lookup-cpp-member/TestCppMemberLocationListLookup.py +++ /dev/null @@ -1,29 +0,0 @@ -"""Test that lldb picks the correct DWARF location list entry with a return-pc out of bounds.""" - -import lldb -from lldbsuite.test.lldbtest import * -from lldbsuite.test import lldbutil - - -class CppMemberLocationListLookupTestCase(TestBase): - def test(self): - self.build() - - exe = self.getBuildArtifact("a.out") - target = self.dbg.CreateTarget(exe) - self.assertTrue(target, VALID_TARGET) - self.dbg.SetAsync(False) - - li = lldb.SBLaunchInfo(["a.out"]) - error = lldb.SBError() - process = target.Launch(li, error) - self.assertTrue(process.IsValid()) - self.assertTrue(process.is_stopped) - - # Find `bar` on the stack, then - # find `this` local variable, then - # check that we can read out the pointer value - for f in process.GetSelectedThread().frames: - if f.GetDisplayFunctionName().startswith("Foo::bar"): - process.GetSelectedThread().SetSelectedFrame(f.idx) - self.expect_expr("this", result_type="Foo *") diff --git a/lldb/test/API/functionalities/location-list-lookup/Makefile b/lldb/test/API/functionalities/location-list-lookup/Makefile index 78b0b11cb7484e..8e453681d7b390 100644 --- a/lldb/test/API/functionalities/location-list-lookup/Makefile +++ b/lldb/test/API/functionalities/location-list-lookup/Makefile @@ -1,3 +1,3 @@ -C_SOURCES := main.c +CXX_SOURCES := main.cpp CFLAGS_EXTRAS := -O1 include Makefile.rules diff --git a/lldb/test/API/functionalities/location-list-lookup/TestLocationListLookup.py b/lldb/test/API/functionalities/location-list-lookup/TestLocationListLookup.py index 4793447c594132..653d8ce150e331 100644 --- a/lldb/test/API/functionalities/location-list-lookup/TestLocationListLookup.py +++ b/lldb/test/API/functionalities/location-list-lookup/TestLocationListLookup.py @@ -1,22 +1,15 @@ """Test that lldb picks the correct DWARF location list entry with a return-pc out of bounds.""" import lldb -from lldbsuite.test.decorators import * from lldbsuite.test.lldbtest import * from lldbsuite.test import lldbutil -class LocationListLookupTestCase(TestBase): - def setUp(self): - # Call super's setUp(). - TestBase.setUp(self) - - @skipIf(oslist=["linux"], archs=["arm"]) - def test_loclist(self): +class CppMemberLocationListLookupTestCase(TestBase): + def test(self): self.build() - exe = self.getBuildArtifact("a.out") - # Create a target by the debugger. + exe = self.getBuildArtifact("a.out") target = self.dbg.CreateTarget(exe) self.assertTrue(target, VALID_TARGET) self.dbg.SetAsync(False) @@ -27,12 +20,15 @@ def test_loclist(self): self.assertTrue(process.IsValid()) self.assertTrue(process.is_stopped) - # Find `main` on the stack, then - # find `argv` local variable, then - # check that we can read the c-string in argv[0] + # Find `bar` on the stack, then + # find `this` local variable, then + # check that we can read out the pointer value for f in process.GetSelectedThread().frames: - if f.GetDisplayFunctionName() == "main": + if f.GetDisplayFunctionName().startswith("Foo::bar"): argv = f.GetValueForVariablePath("argv").GetChildAtIndex(0) strm = lldb.SBStream() argv.GetDescription(strm) self.assertNotEqual(strm.GetData().find("a.out"), -1) + + process.GetSelectedThread().SetSelectedFrame(f.idx) + self.expect_expr("this", result_type="Foo *") diff --git a/lldb/test/API/functionalities/location-list-lookup/main.c b/lldb/test/API/functionalities/location-list-lookup/main.c deleted file mode 100644 index 852772ee52ca2d..00000000000000 --- a/lldb/test/API/functionalities/location-list-lookup/main.c +++ /dev/null @@ -1,23 +0,0 @@ -#include <stdio.h> -#include <stdlib.h> - -// The goal with this test is: -// 1. Have main() followed by foo() -// 2. Have the no-return call to abort() in main be the last instruction -// 3. Have the next instruction be the start of foo() -// 4. The debug info for argv uses a location list. -// clang at -O1 on x86_64 or arm64 has debuginfo like -// DW_AT_location (0x00000049: -// [0x0000000100003f15, 0x0000000100003f25): DW_OP_reg4 RSI -// [0x0000000100003f25, 0x0000000100003f5b): DW_OP_reg15 R15) - -void foo(int); -int main(int argc, char **argv) { - char *file = argv[0]; - char f0 = file[0]; - printf("%c\n", f0); - foo(f0); - printf("%s %d\n", argv[0], argc); - abort(); /// argv is still be accessible here -} -void foo(int in) { printf("%d\n", in); } diff --git a/lldb/test/API/functionalities/location-list-lookup-cpp-member/main.cpp b/lldb/test/API/functionalities/location-list-lookup/main.cpp similarity index 68% rename from lldb/test/API/functionalities/location-list-lookup-cpp-member/main.cpp rename to lldb/test/API/functionalities/location-list-lookup/main.cpp index 96817f141b82cb..4ccdadbddbb555 100644 --- a/lldb/test/API/functionalities/location-list-lookup-cpp-member/main.cpp +++ b/lldb/test/API/functionalities/location-list-lookup/main.cpp @@ -5,18 +5,18 @@ void func(int in); struct Foo { int x; - [[clang::noinline]] void bar(Foo *f); + [[clang::noinline]] void bar(char **argv); }; int main(int argc, char **argv) { Foo f{.x = 5}; std::printf("%p\n", &f.x); - f.bar(&f); + f.bar(argv); return f.x; } -void Foo::bar(Foo *f) { - std::printf("%p %p\n", f, this); +void Foo::bar(char **argv) { + std::printf("%p %p\n", argv, this); std::abort(); /// 'this' should be still accessible } _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
