[lldb] Support stepping through C++ thunks #127419
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR fixes LLDB stepping out, rather than stepping through a C++ thunk. The implementation is based on, and upstreams, the support for runtime thunks in the Swift fork. Fixes llvm#43413
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesThis PR fixes LLDB stepping out, rather than stepping through a C++ thunk. The implementation is based on, and upstreams, the support for runtime thunks in the Swift fork. Fixes #43413 Full diff: https://github.com/llvm/llvm-project/pull/127419.diff 7 Files Affected:
diff --git a/lldb/include/lldb/Target/LanguageRuntime.h b/lldb/include/lldb/Target/LanguageRuntime.h
index f9ae2dc589632..7e4c11df0da7f 100644
--- a/lldb/include/lldb/Target/LanguageRuntime.h
+++ b/lldb/include/lldb/Target/LanguageRuntime.h
@@ -201,6 +201,8 @@ class LanguageRuntime : public Runtime, public PluginInterface {
return false;
}
+ virtual bool IsSymbolARuntimeThunk(const Symbol &symbol) { return false; }
+
// Given the name of a runtime symbol (e.g. in Objective-C, an ivar offset
// symbol), try to determine from the runtime what the value of that symbol
// would be. Useful when the underlying binary is stripped.
diff --git a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
index 42fa54634841c..e648b9665bef6 100644
--- a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
+++ b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
@@ -476,3 +476,9 @@ CPPLanguageRuntime::GetStepThroughTrampolinePlan(Thread &thread,
return ret_plan_sp;
}
+
+bool CPPLanguageRuntime::IsSymbolARuntimeThunk(const Symbol &symbol) {
+ llvm::outs() << symbol.GetMangled().GetMangledName().GetStringRef() << '\n';
+ return symbol.GetMangled().GetMangledName().GetStringRef().starts_with(
+ "_ZThn");
+}
diff --git a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.h b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.h
index 57cfe28245808..05639e9798917 100644
--- a/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.h
+++ b/lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.h
@@ -78,6 +78,9 @@ class CPPLanguageRuntime : public LanguageRuntime {
bool stop_others) override;
bool IsAllowedRuntimeValue(ConstString name) override;
+
+ bool IsSymbolARuntimeThunk(const Symbol &symbol) override;
+
protected:
// Classes that inherit from CPPLanguageRuntime can see and modify these
CPPLanguageRuntime(Process *process);
diff --git a/lldb/source/Target/ThreadPlanShouldStopHere.cpp b/lldb/source/Target/ThreadPlanShouldStopHere.cpp
index e72f8d8f51a20..723d7965c3467 100644
--- a/lldb/source/Target/ThreadPlanShouldStopHere.cpp
+++ b/lldb/source/Target/ThreadPlanShouldStopHere.cpp
@@ -8,6 +8,7 @@
#include "lldb/Target/ThreadPlanShouldStopHere.h"
#include "lldb/Symbol/Symbol.h"
+#include "lldb/Target/LanguageRuntime.h"
#include "lldb/Target/RegisterContext.h"
#include "lldb/Target/Thread.h"
#include "lldb/Utility/LLDBLog.h"
@@ -76,6 +77,18 @@ bool ThreadPlanShouldStopHere::DefaultShouldStopHereCallback(
}
}
+ // Check whether the frame we are in is a language runtime thunk, only for
+ // step out:
+ if (operation == eFrameCompareOlder) {
+ Symbol *symbol = frame->GetSymbolContext(eSymbolContextSymbol).symbol;
+ if (symbol) {
+ ProcessSP process_sp(current_plan->GetThread().GetProcess());
+ for (auto *runtime : process_sp->GetLanguageRuntimes()) {
+ if (runtime->IsSymbolARuntimeThunk(*symbol))
+ should_stop_here = false;
+ }
+ }
+ }
// Always avoid code with line number 0.
// FIXME: At present the ShouldStop and the StepFromHere calculate this
// independently. If this ever
@@ -109,18 +122,34 @@ ThreadPlanSP ThreadPlanShouldStopHere::DefaultStepFromHereCallback(
if (sc.line_entry.line == 0) {
AddressRange range = sc.line_entry.range;
-
- // If the whole function is marked line 0 just step out, that's easier &
- // faster than continuing to step through it.
bool just_step_out = false;
- if (sc.symbol && sc.symbol->ValueIsAddress()) {
- Address symbol_end = sc.symbol->GetAddress();
- symbol_end.Slide(sc.symbol->GetByteSize() - 1);
- if (range.ContainsFileAddress(sc.symbol->GetAddress()) &&
- range.ContainsFileAddress(symbol_end)) {
- LLDB_LOGF(log, "Stopped in a function with only line 0 lines, just "
- "stepping out.");
- just_step_out = true;
+ if (sc.symbol) {
+ ProcessSP process_sp(current_plan->GetThread().GetProcess());
+
+ // If this is a runtime thunk, step through it, rather than stepping out
+ // because it's marked line 0.
+ bool is_thunk = false;
+ for (auto *runtime : process_sp->GetLanguageRuntimes()) {
+ if (runtime->IsSymbolARuntimeThunk(*sc.symbol)) {
+ LLDB_LOGF(log, "In runtime thunk %s - stepping out.",
+ sc.symbol->GetName().GetCString());
+ is_thunk = true;
+ }
+ }
+
+ // If the whole function is marked line 0 just step out, that's easier &
+ // faster than continuing to step through it.
+ // FIXME: This assumes that the function is a single line range. It could
+ // be a series of contiguous line 0 ranges. Check for that too.
+ if (!is_thunk && sc.symbol->ValueIsAddress()) {
+ Address symbol_end = sc.symbol->GetAddress();
+ symbol_end.Slide(sc.symbol->GetByteSize() - 1);
+ if (range.ContainsFileAddress(sc.symbol->GetAddress()) &&
+ range.ContainsFileAddress(symbol_end)) {
+ LLDB_LOGF(log, "Stopped in a function with only line 0 lines, just "
+ "stepping out.");
+ just_step_out = true;
+ }
}
}
if (!just_step_out) {
diff --git a/lldb/test/API/lang/cpp/thunk/Makefile b/lldb/test/API/lang/cpp/thunk/Makefile
new file mode 100644
index 0000000000000..99998b20bcb05
--- /dev/null
+++ b/lldb/test/API/lang/cpp/thunk/Makefile
@@ -0,0 +1,3 @@
+CXX_SOURCES := main.cpp
+
+include Makefile.rules
diff --git a/lldb/test/API/lang/cpp/thunk/TestThunk.py b/lldb/test/API/lang/cpp/thunk/TestThunk.py
new file mode 100644
index 0000000000000..bd17401f1d935
--- /dev/null
+++ b/lldb/test/API/lang/cpp/thunk/TestThunk.py
@@ -0,0 +1,24 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class ThunkTest(TestBase):
+ def test(self):
+ self.build()
+ lldbutil.run_to_name_breakpoint(self, "testit")
+
+ self.expect(
+ "step",
+ STEP_IN_SUCCEEDED,
+ substrs=["stop reason = step in", "Derived1::doit"],
+ )
+
+ self.runCmd("continue")
+
+ self.expect(
+ "step",
+ STEP_IN_SUCCEEDED,
+ substrs=["stop reason = step in", "Derived2::doit"],
+ )
diff --git a/lldb/test/API/lang/cpp/thunk/main.cpp b/lldb/test/API/lang/cpp/thunk/main.cpp
new file mode 100644
index 0000000000000..36f2d7c2ce421
--- /dev/null
+++ b/lldb/test/API/lang/cpp/thunk/main.cpp
@@ -0,0 +1,36 @@
+#include <stdio.h>
+
+class Base1 {
+public:
+ virtual ~Base1() {}
+};
+
+class Base2 {
+public:
+ virtual void doit() = 0;
+};
+
+Base2 *b = nullptr;
+
+class Derived1 : public Base1, public Base2 {
+public:
+ virtual void doit() { printf("Derived1\n"); }
+};
+
+class Derived2 : public Base2 {
+public:
+ virtual void doit() { printf("Derived2\n"); }
+};
+
+void testit() { b->doit(); }
+
+int main() {
+
+ b = new Derived1();
+ testit();
+
+ b = new Derived2();
+ testit();
+
+ return 0;
+}
|
JDevlieghere
changed the title
Michael137
reviewed
Feb 17, 2025
lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
Outdated
Show resolved
Hide resolved
Michael137
reviewed
Feb 17, 2025
lldb/source/Plugins/LanguageRuntime/CPlusPlus/CPPLanguageRuntime.cpp
Outdated
Show resolved
Hide resolved
|
I'm the one who recently added an example reproducing the #43413 issue. I compiled this PR version of lldb and related tools and tested it against the simple reproduction I added to the issue, and also with our complex project where I first faced this issue. In both cases lldb works as expected with this patch. Thank you very much! |
|
Makes sense to me but I'll defer to @jimingham and co. for the stepping bits |
|
@JDevlieghere would you mind getting rid of the nullptr in the testcase, main.cpp line 13? *b is set to nullptr per language standard, and those of use stuck in c++03 on embedded targets would appreciate it! |
Michael137
reviewed
Feb 17, 2025
| // non-virtual base, with fixed this adjustments, use a "Th" prefix and encode | ||
| // the required adjustment offset, probably negative, indicated by a 'n' | ||
| // prefix, and the encoding of the target function. | ||
| return symbol.GetMangled().GetMangledName().GetStringRef().starts_with( |
There was a problem hiding this comment.
Hmm i think we also want to check Tc and Tv?
Tv you can test by inheriting one of the bases virtually. And here's an example of a Tc encoding:
struct V1 { };
struct V2 : virtual V1 { };
struct A {
virtual V1 *f();
};
struct B : A {
virtual V2 *f();
};
V2 *B::f() { return 0; }
$ c++filt -n _ZTch0_v0_n24_N1B1fEv
covariant return thunk to B::f()
There was a problem hiding this comment.
I should've read the whole paragraph :-)
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
This looks fine. The one thing that we might want to also test is that if we step through the thunk, and then stop in some code that we don't want to stop in (i.e. the target of the thunk doesn't have debug information) we step back out correctly. This should work by composition, but thunks can be funny and harder to trace out of... I don't remember whether we had tests for this on the swift side, but it shouldn't be too hard to have the target of the thunk in a separate .cpp file, and strip that one. Then make sure we don't leave the user stranded there. |
medismailben
approved these changes
Feb 17, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/6383 Here is the relevant piece of the build log for the reference |
JDevlieghere
added a commit
that referenced
this pull request
Feb 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes LLDB stepping out, rather than stepping through a C++ thunk. The implementation is based on, and upstreams, the support for runtime thunks in the Swift fork.
Fixes #43413