-
Notifications
You must be signed in to change notification settings - Fork 321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[lldb][swift] Filter await-resume funclets when setting breakpoints #8319
base: stable/20230725
Are you sure you want to change the base?
[lldb][swift] Filter await-resume funclets when setting breakpoints #8319
Conversation
I'm very open to other ways of accomplishing this, in case anyone has suggestions |
Filtering them in the File & Line resolver seems fine to me. However, there needs to be a |
You should probably also do the generic stuff on the llvm.org version, cherry-pick that and add your implementation so we don't end up with more diffs on the swift side. |
I actually had that on my first iteration of this patch, but then realized we would be putting virtual methods with no real implementation upstream, which is generally something that sees push back against (it would basically be dead code, since Swift is not an upstreamed language). If you believe this won't be a problem, I can go the language route (still need to figure out if we have access to a |
This is a general feature, where the compiler is emitting some debug information that for its own internal reasons it wishes it could manage not to emit, but can't. OTOH, the debugger after the fact can. There's nothing swift specific about that, except currently the swift compiler is the only one that does it. So I don't think it will be an issue putting in an API here - and that's much preferable to more The breakpoints offer an option to filter by language, so you should be able to get your hands on the language of the function you are setting the breakpoint in. |
075ab30
to
1acab5d
Compare
Sounds good, I will convert this to a draft while I propose the first commit of this PR upstream |
Upstream patch llvm#83908 |
lldb/include/lldb/Target/Language.h
Outdated
@@ -337,6 +337,10 @@ class Language : public PluginInterface { | |||
|
|||
virtual llvm::StringRef GetInstanceVariableName() { return {}; } | |||
|
|||
virtual bool IsInterestingCtxForLineBreakpoint(const SymbolContext &) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doxygen comment?
Also we can probably spell out Ctx?
return false; | ||
}); | ||
} | ||
|
||
void BreakpointResolver::SetSCMatchesByLine( | ||
SearchFilter &filter, SymbolContextList &sc_list, bool skip_prologue, | ||
llvm::StringRef log_ident, uint32_t line, std::optional<uint16_t> column) { | ||
llvm::SmallVector<SymbolContext, 16> all_scs; | ||
for (uint32_t i = 0; i < sc_list.GetSize(); ++i) | ||
all_scs.push_back(sc_list[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just apply the filter right here before entering it into the list?
1acab5d
to
05ca723
Compare
I've re-implemented the swift side of things with the suggestions and marked this patch as ready for review. Note that the first commit here is identical to https://github.com/llvm/llvm-project/pull/83908/files, and I'll convert it to an actual cherry-pick once it gets merged upstream (won't merge this PR until that's done) |
ec8fd7a
to
965305d
Compare
lldb/include/lldb/Target/Language.h
Outdated
// by line (number or regex). This is useful for languages that create | ||
// artificial functions without any meaningful user code associated with them | ||
// (e.g. code that gets expanded in late compilation stages, like by | ||
// CoroSplitter). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
///
lldb/include/lldb/Target/Language.h
Outdated
// artificial functions without any meaningful user code associated with them | ||
// (e.g. code that gets expanded in late compilation stages, like by | ||
// CoroSplitter). | ||
virtual bool IsArtificialCtxForLineBreakpoint(const SymbolContext &) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about IgnoreForLineBreakpoints
?
->GetTarget() | ||
.GetIgnoreBreakpointsFromLanguageArtificialLocations(); | ||
|
||
for (uint32_t i = 0; i < sc_list.GetSize(); ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be range-based?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can, I just didn't want to add to the diff. As much as github is saying this is new code, it's not new code...
StringRef name = sc.function->GetMangled().GetMangledName().GetStringRef(); | ||
// In async functions, ignore "await resume" funclets, these only deallocate | ||
// the async context and task_switch back to user code. | ||
return !SwiftLanguageRuntime::IsSwiftAsyncAwaitResumePartialFunctionSymbol(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
@adrian-prantl I've addressed your comments on the upstream pr llvm#83908 |
965305d
to
0d362b6
Compare
Some languages may create artificial functions that have no real user code, even though there is line table information for them. One such case is with coroutine code that receives the CoroSplitter transformation in LLVM IR. That code transformation creates many different Functions, cloning one Instruction into many Instructions in many different Functions and copying the associated debug locations. It would be difficult to make that pass delete debug locations of cloned instructions in a language agnostic way (is it even possible?), but LLDB can ignore certain locations by querying its Language APIs and having it decide based on, for example, mangling information. (cherry picked from commit 0adccd1)
These funclets only serve to `task_dealloc` previously allocated tasks when returning from an async call, and immediately `task_switch` to the next await-suspend funclet (which contains real user code). By not filtering out these funclets, any breakpoint on a line with an async call will cause execution to pause 3 times: once before the call, twice when "returning" from the call, which makes for a confusing experience. The patch does the filtering on `BreakpointResolver::SetSCMatchesByLine`, which is the common code between BreakpointResolverFileLine and BreakpointResolverFileRegex. We also considered changing the debug line information in any of the many different lowering stages the swift compiler, but this turned out to be very complex to do in a targeted way; more often than not, a handful of early-IR coroutine instructions get expanded into multiple function clones, all inheriting the same debug line information. The current approach also has the advantaged of being easily reversible if we decide to do so.
0d362b6
to
b98b5a3
Compare
Now with the official cherry-pick of the upstream lldb commit. |
@swift-ci test |
@@ -360,3 +360,9 @@ let Definition = "thread" in { | |||
DefaultUnsignedValue<600000>, | |||
Desc<"Maximum number of frames to backtrace.">; | |||
} | |||
|
|||
let Definition = "language" in { | |||
def EnableFilterForLineBreakpoints: Property<"enable-filter-for-line-breakpoints", "Boolean">, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a naming convention around boolean properties? Should this be prefixed with enable-, or not? It seems possibly redundant to me. Maybe the name should be filter-for-line-breakpoints
and the values enable
and disable
? I'm not strongly opposed to the name, but it did strike me as a little unconventional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current practice has all "enable"-like properties using boolean values, and most of them mentioning "enable" in the actual setting flag, though there are exceptions to this second point
These funclets only serve to
task_dealloc
previously allocated tasks when returning from an async call, and immediatelytask_switch
to the next await-suspend funclet (which contains real user code).By not filtering out these funclets, any breakpoint on a line with an async call will cause execution to pause 3 times: once before the call, twice when "returning" from the call, which makes for a confusing experience.
The patch does the filtering on
BreakpointResolver::SetSCMatchesByLine
, which is the common code between BreakpointResolverFileLine and BreakpointResolverFileRegex.We also considered changing the debug line information in any of the many different lowering stages the swift compiler, but this turned out to be very complex to do in a targeted way; more often than not, a handful of early-IR coroutine instructions get expanded into multiple function clones, all inheriting the same debug line information. The current approach also has the advantaged of being easily reversible if we decide to do so.