[lldb][Symbol] Make sure we decrement PC before checking location list

[Michael137]

When stopping in optimized code, we can end up with the 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 addressed 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.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

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

[jasonmolenda]

This looks good to me. Do you think we take the frame variable test in API/functionalities/location-list-lookup/TestLocationListLookup.py and add it to your test? They're testing the same thing, and getting there the same way so I'm not sure there's value to having the two separate tests for the frame variable codepath and the expr codepath.

[Michael137]

This looks good to me. Do you think we take the frame variable test in API/functionalities/location-list-lookup/TestLocationListLookup.py and add it to your test? They're testing the same thing, and getting there the same way so I'm not sure there's value to having the two separate tests for the frame variable codepath and the expr codepath.

Yup makes sense!