[lldb][swift] Filter await-resume funclets when setting breakpoints #8319

felipepiovezan · 2024-03-02T01:12:51Z

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.

felipepiovezan · 2024-03-02T01:13:20Z

I'm very open to other ways of accomplishing this, in case anyone has suggestions

jimingham · 2024-03-02T01:38:37Z

Filtering them in the File & Line resolver seems fine to me.

However, there needs to be a virtual Language::RemoveInvalidSourceLineMatches or something like that which does nothing. Then your function needs to implement that in the Swift Language. I'm not wedded to the name, but you have to make this generic. We really want to keep from adding more new #ifdef LLDB_ENABLE_SWIFT in generic files.

jimingham · 2024-03-02T01:39:47Z

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.

felipepiovezan · 2024-03-02T04:22:16Z

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 Language object from this point)

jimingham · 2024-03-04T17:05:56Z

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 #ifdef LLDB_ENABLE_SWIFT.

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.

felipepiovezan · 2024-03-04T21:01:25Z

Sounds good, I will convert this to a draft while I propose the first commit of this PR upstream

felipepiovezan · 2024-03-04T21:08:18Z

Upstream patch llvm#83908

adrian-prantl · 2024-03-04T22:50:03Z

lldb/include/lldb/Target/Language.h

@@ -337,6 +337,10 @@ class Language : public PluginInterface {

 virtual llvm::StringRef GetInstanceVariableName() { return {}; }

+ virtual bool IsInterestingCtxForLineBreakpoint(const SymbolContext &) const {


Doxygen comment?
Also we can probably spell out Ctx?

adrian-prantl · 2024-03-04T22:51:23Z

lldb/source/Breakpoint/BreakpointResolver.cpp

+ 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]);


Why not just apply the filter right here before entering it into the list?

lldb/source/Plugins/Language/Swift/SwiftLanguage.cpp

felipepiovezan · 2024-03-06T16:54:59Z

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)

adrian-prantl · 2024-03-06T17:35:03Z

lldb/include/lldb/Target/Language.h

+ // 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).


adrian-prantl · 2024-03-06T17:36:26Z

lldb/include/lldb/Target/Language.h

+ // 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 {


how about IgnoreForLineBreakpoints ?

adrian-prantl · 2024-03-06T17:36:59Z

lldb/source/Breakpoint/BreakpointResolver.cpp

+ ->GetTarget()
+ .GetIgnoreBreakpointsFromLanguageArtificialLocations();
+
+ for (uint32_t i = 0; i < sc_list.GetSize(); ++i) {


can this be range-based?

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...

adrian-prantl · 2024-03-06T17:37:22Z

lldb/source/Plugins/Language/Swift/SwiftLanguage.cpp

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


felipepiovezan · 2024-03-07T17:33:57Z

@adrian-prantl I've addressed your comments on the upstream pr llvm#83908

lldb/include/lldb/Target/Language.h

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.

felipepiovezan · 2024-03-14T16:54:36Z

Now with the official cherry-pick of the upstream lldb commit.

felipepiovezan · 2024-03-14T16:54:43Z

@swift-ci test

kastiglione · 2024-03-14T18:29:06Z

lldb/source/Target/TargetProperties.td

@@ -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">,


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.

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

felipepiovezan requested review from kastiglione, adrian-prantl and jimingham March 2, 2024 01:12

felipepiovezan force-pushed the felipe/filter_async_funclets branch from 075ab30 to 1acab5d Compare March 4, 2024 21:00

felipepiovezan marked this pull request as draft March 4, 2024 21:01

adrian-prantl reviewed Mar 4, 2024

View reviewed changes

felipepiovezan force-pushed the felipe/filter_async_funclets branch from 1acab5d to 05ca723 Compare March 6, 2024 16:49

felipepiovezan requested a review from adrian-prantl March 6, 2024 16:53

felipepiovezan marked this pull request as ready for review March 6, 2024 16:53

felipepiovezan force-pushed the felipe/filter_async_funclets branch 2 times, most recently from ec8fd7a to 965305d Compare March 6, 2024 17:08

adrian-prantl reviewed Mar 6, 2024

View reviewed changes

felipepiovezan force-pushed the felipe/filter_async_funclets branch from 965305d to 0d362b6 Compare March 7, 2024 17:43

adrian-prantl approved these changes Mar 8, 2024

View reviewed changes

lldb/include/lldb/Target/Language.h Show resolved Hide resolved

felipepiovezan added 2 commits March 14, 2024 09:48

felipepiovezan force-pushed the felipe/filter_async_funclets branch from 0d362b6 to b98b5a3 Compare March 14, 2024 16:53

adrian-prantl approved these changes Mar 14, 2024

View reviewed changes

kastiglione reviewed Mar 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb][swift] Filter await-resume funclets when setting breakpoints #8319

[lldb][swift] Filter await-resume funclets when setting breakpoints #8319

felipepiovezan commented Mar 2, 2024

felipepiovezan commented Mar 2, 2024

jimingham commented Mar 2, 2024

jimingham commented Mar 2, 2024

felipepiovezan commented Mar 2, 2024 •

edited

jimingham commented Mar 4, 2024

felipepiovezan commented Mar 4, 2024

felipepiovezan commented Mar 4, 2024

adrian-prantl Mar 4, 2024

adrian-prantl Mar 4, 2024

felipepiovezan commented Mar 6, 2024

adrian-prantl Mar 6, 2024

adrian-prantl Mar 6, 2024

adrian-prantl Mar 6, 2024

felipepiovezan Mar 6, 2024

adrian-prantl Mar 6, 2024

felipepiovezan commented Mar 7, 2024

felipepiovezan commented Mar 14, 2024

felipepiovezan commented Mar 14, 2024

kastiglione Mar 14, 2024

felipepiovezan Mar 14, 2024 •

edited

		@@ -337,6 +337,10 @@ class Language : public PluginInterface {

		virtual llvm::StringRef GetInstanceVariableName() { return {}; }

		virtual bool IsInterestingCtxForLineBreakpoint(const SymbolContext &) const {

[lldb][swift] Filter await-resume funclets when setting breakpoints #8319

Are you sure you want to change the base?

[lldb][swift] Filter await-resume funclets when setting breakpoints #8319

Conversation

felipepiovezan commented Mar 2, 2024

felipepiovezan commented Mar 2, 2024

jimingham commented Mar 2, 2024

jimingham commented Mar 2, 2024

felipepiovezan commented Mar 2, 2024 • edited

jimingham commented Mar 4, 2024

felipepiovezan commented Mar 4, 2024

felipepiovezan commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipepiovezan commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipepiovezan commented Mar 7, 2024

felipepiovezan commented Mar 14, 2024

felipepiovezan commented Mar 14, 2024

Choose a reason for hiding this comment

felipepiovezan Mar 14, 2024 • edited

Choose a reason for hiding this comment

felipepiovezan commented Mar 2, 2024 •

edited

felipepiovezan Mar 14, 2024 •

edited