https://github.com/Michael137 created https://github.com/llvm/llvm-project/pull/74772
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. >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] [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 85ceadd20c611..db740cb7cb6e4 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 0000000000000..8e453681d7b39 --- /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 0000000000000..846386ceffc6a --- /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 0000000000000..96817f141b82c --- /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); } _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
