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

[Michael Buch via lldb-commits]

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