[Lldb-commits] [lldb] [lldb][Symbol] Make sure we decrement PC before checking location list (PR #74772)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

<details>
<summary>Changes</summary>

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.

---
Full diff: https://github.com/llvm/llvm-project/pull/74772.diff


4 Files Affected:

- (modified) lldb/source/Symbol/Variable.cpp (+2-1) 
- (added) lldb/test/API/functionalities/location-list-lookup-cpp-member/Makefile (+3) 
- (added) lldb/test/API/functionalities/location-list-lookup-cpp-member/TestCppMemberLocationListLookup.py (+29) 
- (added) lldb/test/API/functionalities/location-list-lookup-cpp-member/main.cpp (+23) 


``````````diff
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); }

``````````

</details>


https://github.com/llvm/llvm-project/pull/74772