[Lldb-commits] [lldb] [lldb][Symbol] Make sure we decrement PC before checking location list (PR #74772)
Michael Buch via lldb-commits
lldb-commits at lists.llvm.org
Thu Dec 7 15:01:59 PST 2023
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 <michaelbuch12 at gmail.com>
Date: Thu, 7 Dec 2023 21:35:41 +0000
Subject: [PATCH 1/3] [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); }
>From 3a56e376861720ef23b4dcc57e99553945777269 Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Thu, 7 Dec 2023 22:54:19 +0000
Subject: [PATCH 2/3] 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 8e453681d7b39..0000000000000
--- 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 846386ceffc6a..0000000000000
--- 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 78b0b11cb7484..8e453681d7b39 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 4793447c59413..653d8ce150e33 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 852772ee52ca2..0000000000000
--- 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 96817f141b82c..4ccdadbddbb55 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
}
>From 3723fc99c6127d1369ea7fc3f6104086fb725413 Mon Sep 17 00:00:00 2001
From: Michael Buch <michaelbuch12 at gmail.com>
Date: Thu, 7 Dec 2023 23:01:33 +0000
Subject: [PATCH 3/3] fixup! combine location-list-lookup tests
---
.../location-list-lookup/TestLocationListLookup.py | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/lldb/test/API/functionalities/location-list-lookup/TestLocationListLookup.py b/lldb/test/API/functionalities/location-list-lookup/TestLocationListLookup.py
index 653d8ce150e33..92b51c4644930 100644
--- a/lldb/test/API/functionalities/location-list-lookup/TestLocationListLookup.py
+++ b/lldb/test/API/functionalities/location-list-lookup/TestLocationListLookup.py
@@ -1,12 +1,14 @@
"""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 CppMemberLocationListLookupTestCase(TestBase):
- def test(self):
+class LocationListLookupTestCase(TestBase):
+ @skipIf(oslist=["linux"], archs=["arm"])
+ def test_loclist(self):
self.build()
exe = self.getBuildArtifact("a.out")
More information about the lldb-commits
mailing list