[lldb] Swift OS plugin #9839

felipepiovezan · 2025-01-15T21:40:04Z

This PR implements the required functionality for a swift OS plugins that converts Tasks into Threads, allowing LLDB to transparently work with Tasks as if they were threads.
In particular, this addresses all (famous last words?) the outstanding stepping issues in swift async code.

lldb/source/Plugins/OperatingSystem/Swift/OperatingSystemSwift.cpp

jimingham · 2025-01-15T22:30:32Z

lldb/source/Plugins/OperatingSystem/Swift/OperatingSystemSwift.cpp

+    }
+
+    // Mask higher bits to avoid conflicts with core thread IDs.
+    uint64_t masked_task_id = 0xdeadbeef00000000 | *task_id;


This will be the visible "TID" for running tasks, right? And what I'd have to type if I wanted to set a "task specific" breakpoint? If so, we might not want it to be deadbeef. That's cute if you know the reference, but otherwise somewhat off-putting...

Oh yeah, I'm actually looking for suggestions here, if we should mask it at all and what to use if we do
Deadbeef was just something to help me grep the logs as I debugged.

jimingham · 2025-01-16T00:45:05Z

lldb/source/Plugins/OperatingSystem/Swift/OperatingSystemSwift.cpp

+    ThreadSP swift_thread = [&]() -> ThreadSP {
+      // If we already had a thread for this Task in the last stop, re-use it.
+      if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);
+          IsOperatingSystemPluginThread(old_thread))


Do you have to handle the case where when you stopped the first time, this Task was backed by "core thread 1" but the next time you stopped it was backed by a different core thread? I don't see where you are changing the backing thread in this case?

I don't see where you are changing the backing thread in this case?

It's just a few lines below, both cases are handled the same way:

swift_thread->SetBackingThread(real_thread);

jimingham · 2025-01-16T00:48:52Z

I note that the only swift specific bits of this are finding the task on a particular thread, and a few checks about whether this should be used that I think belong more properly in the LanguageRuntime that owns the OS plugin...

Would it be possible to make an OperatingSystemTaskOverlay that has all the generic algorithms, and then a swift version that just adds the "how to find the task ID"?. The concept is pretty general, and we could probably mock it with an artificial OS plugin so we could test a lot of this w/o having to involve Swift...

felipepiovezan · 2025-01-24T03:00:02Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntimeNames.cpp

@@ -302,6 +302,7 @@ class ThreadPlanRunToAddressOnAsyncCtx : public ThreadPlan {
    auto &target = thread.GetProcess()->GetTarget();
    m_funclet_bp = target.CreateBreakpoint(destination_addr, true, false);
    m_funclet_bp->SetBreakpointKind("async-run-to-funclet");
+    m_funclet_bp->SetThreadID(thread.GetID());


to refresh reviewer's memories: this is the thread plan to "run through a task switch and stop when we schedule this same async function again. This plan now sets thread specific breakpoints, since they are now task breakpoints!

If we somehow push this type of plan without having the OS plugin, we are at a risk of breaking in the wrong task. But if the plugin is not there and there are tasks, we are going to have much bigger problems anyway. For example, this code has been associated with a lot of LLDB crashes because it triggers stack plan migration (this is a downstream-only thing that becomes dead code with this patch, and I'll delete it in a follow up PR)

Should be this an inline comment in the source file ?

I would argue that in this case we don't need any comments, because this is just the "natural" thing to do. If anything, it is the old implementation that deserved a comment, as it was essentially making all threads stop.

This plan now sets thread specific breakpoints, since they are now task breakpoints!

I'm not sure I understand what you're saying with this.

I just noticed that the writing was indeed confusing, my bad.
The important thing to note here is that this plan is no longer special, it should work like any other plan: when it sets a breakpoint, it sets a thread specific breakpoint (otherwise it would cause all threads to stop).

A follow up patch will remove this plan in favor of the RunToBreakpointPlan

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp

medismailben

cool stuff! I left some comments / suggestions :)

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp

medismailben · 2025-02-04T17:49:18Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp

@@ -168,6 +191,52 @@ static ModuleSP findRuntime(Process &process, RuntimeKind runtime_kind) {
  return runtime_image;
 }

+ModuleSP SwiftLanguageRuntime::findConcurrencyModule(Process &process) {


nit: casing

Suggested change

ModuleSP SwiftLanguageRuntime::findConcurrencyModule(Process &process) {

ModuleSP SwiftLanguageRuntime::FindConcurrencyModule(Process &process) {

medismailben · 2025-02-04T17:49:34Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp

+}
+
+std::optional<uint32_t>
+SwiftLanguageRuntime::findConcurrencyDebugVersion(Process &process) {


oops, will change!

medismailben · 2025-02-04T17:49:47Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.h

@@ -98,6 +98,16 @@ class SwiftLanguageRuntime : public LanguageRuntime {
  static SwiftLanguageRuntime *Get(lldb::ProcessSP process_sp) {
    return SwiftLanguageRuntime::Get(process_sp.get());
  }
+
+  /// Returns the Module containing the Swift Concurrency runtime, if it exists.
+  static lldb::ModuleSP findConcurrencyModule(Process &process);


medismailben · 2025-02-04T17:50:02Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.h

+  /// returned.
+  /// Returns 0 for versions of the module prior to the introduction
+  /// of versioning.
+  static std::optional<uint32_t> findConcurrencyDebugVersion(Process &process);


medismailben · 2025-02-04T17:51:06Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntimeNames.cpp

@@ -302,6 +302,7 @@ class ThreadPlanRunToAddressOnAsyncCtx : public ThreadPlan {
    auto &target = thread.GetProcess()->GetTarget();
    m_funclet_bp = target.CreateBreakpoint(destination_addr, true, false);
    m_funclet_bp->SetBreakpointKind("async-run-to-funclet");
+    m_funclet_bp->SetThreadID(thread.GetID());


Should be this an inline comment in the source file ?

medismailben · 2025-02-04T17:53:15Z

lldb/source/Plugins/OperatingSystem/SwiftTasks/OperatingSystemSwiftTasks.cpp

+    }
+
+    // Mask higher bits to avoid conflicts with core thread IDs.
+    uint64_t masked_task_id = 0x0000000f00000000 | *task_id;


Nice hack :p

To be honest I don't think we need to / should do this, because conflicts are handled without issues... it is just a way to visually know we have a fake thread.

how are the conflicts handled?

It's a couple of lines down, on line 100

medismailben · 2025-02-04T17:55:14Z

lldb/source/Plugins/OperatingSystem/SwiftTasks/OperatingSystemSwiftTasks.cpp

+      if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);
+          IsOperatingSystemPluginThread(old_thread))


First time I'm seeing this syntax :p Don't you want to run the second part of the if condition only if the first one if valid ?

Suggested change

if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);

IsOperatingSystemPluginThread(old_thread))

if ((ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id)) && IsOperatingSystemPluginThread(old_thread))

IsOperatingSystemPluginThread is designed to take a pointer argument, so it knows how to handle nullptrs.

felipepiovezan · 2025-02-04T21:52:29Z

addressed review comments

kastiglione · 2025-02-04T22:28:14Z

lldb/source/Plugins/OperatingSystem/SwiftTasks/OperatingSystemSwiftTasks.cpp

+  size_t ptr_size = process.GetAddressByteSize();
+  // Offset of a Task ID inside a Task data structure, guaranteed by the ABI.
+  // See Job in swift/RemoteInspection/RuntimeInternals.h.
+  m_task_id_offset = 4 * ptr_size + 4;


is this the job id? if so should the name reflect that? also if so, do we know if we can ignore the task's upper 32 bit identifier?

Based on what I saw in the runtime code, I think it's safe to do so. It is what is being done in other tools anyway.

I'll rename this to "job id"!

kastiglione · 2025-02-04T22:41:38Z

lldb/source/Plugins/OperatingSystem/SwiftTasks/OperatingSystemSwiftTasks.cpp

+    ThreadSP swift_thread = [&]() -> ThreadSP {
+      // If we already had a thread for this Task in the last stop, re-use it.
+      if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);
+          IsOperatingSystemPluginThread(old_thread))
+        return old_thread;
+
+      std::string name = llvm::formatv("Swift Task {0:x}", *task_id);
+      llvm::StringRef queue_name = "";
+      return std::make_shared<ThreadMemory>(*m_process, masked_task_id, name,
+                                            queue_name,
+                                            /*register_data_addr*/ 0);
+    }();


The lambda through me off. Is there something I'm missing that it can't be an if/else?

Suggested change

ThreadSP swift_thread = [&]() -> ThreadSP {

// If we already had a thread for this Task in the last stop, re-use it.

if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);

IsOperatingSystemPluginThread(old_thread))

return old_thread;

std::string name = llvm::formatv("Swift Task {0:x}", *task_id);

llvm::StringRef queue_name = "";

return std::make_shared<ThreadMemory>(*m_process, masked_task_id, name,

queue_name,

/*register_data_addr*/ 0);

}();

ThreadSP swift_thread;

// If we already had a thread for this Task in the last stop, re-use it.

if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);

IsOperatingSystemPluginThread(old_thread)) {

swift_thread = old_thread;

} else {

std::string name = llvm::formatv("Swift Task {0:x}", *task_id);

llvm::StringRef queue_name = "";

swift_thread = std::make_shared<ThreadMemory>(*m_process, masked_task_id, name,

queue_name,

/*register_data_addr*/ 0);

}

The missing piece is that this creates an uninitialized variable :)
I'll move this code to a helper function

jimingham · 2025-02-05T18:25:10Z

lldb/source/Plugins/OperatingSystem/SwiftTasks/OperatingSystemSwiftTasks.cpp

+
+  if (*concurrency_version <= 1)
+    return new OperatingSystemSwiftTasks(*process);
+  LLDB_LOGF(log,


Did you intend to only log the version in the failure case?

Also, if you've encountered a more recent version of the concurrency library that this lldb recognizes, we should tell the user that. Could there be a Status path threaded to this Initialize.

Yes, I was originally only logging the problematic case, but there is no harm in logging it all the time, so I'll move the log statement before the conditional

Also, if you've encountered a more recent version of the concurrency library that this lldb recognizes, we should tell the user that. Could there be a Status path threaded to this Initialize.

Is this how we do user warnings? I was grepping around and found Debugger::ReportWarning, with a matching std::once_flag flag. If this is the standard way of doing things, and to make sure a warning for this plugin is only reported once, it seems like the implementation would be:

call ReportWarning inside OperatingSystemSwiftTasks directly,

have that class own the std::once flag. (Note that the "CreateInstance" methods are called many times, e.g. every time libraries are loaded)

What you've added here looks right. We only warn if we found the concurrence library but it looks funny to us.

jimingham · 2025-02-05T18:30:54Z

lldb/source/Plugins/OperatingSystem/SwiftTasks/OperatingSystemSwiftTasks.cpp

+        return old_thread;
+
+      std::string name = llvm::formatv("Swift Task {0:x}", *task_id);
+      llvm::StringRef queue_name = "";


If the core thread had a queue name, would we want to use it here?

Just to register an offline conversation, an empty queue name will make ThreadMemory propagate the backing thread queue name once this fix gets merged: llvm#126128

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp

jimingham

This LGTM. This worked more easily than I expected it would, that's great.

I don't see why the tests shouldn't also work on Linux/Windows but we don't need to hold up this patch to resolve that.

medismailben

LGTM!

felipepiovezan · 2025-02-07T16:30:57Z

@swift-ci test

felipepiovezan · 2025-02-07T17:28:13Z

Update the commit message

felipepiovezan · 2025-02-07T17:28:17Z

@swift-ci test

felipepiovezan · 2025-02-10T17:59:30Z

@swift-ci test

…version

This is here mostly out of an abundance of caution, the default is to use the plugins.

This commit introduces an OS plugin converting Process threads into OperatingSystem threads that represent a Task. This is done by inspecting ThreadLocalStorage for a specific location where the swift runtime writes information about the current Task. With this plugin, ThreadPlans now work properly in async code: a step action on a Thread is now effectively an action on a Task, and LLDB will correctly track that. ThreadPlans are effectively "Task Plans".

felipepiovezan · 2025-02-10T18:00:19Z

Rebased

felipepiovezan · 2025-02-10T18:00:28Z

@swift-ci test

With [1], stepping should work reliably even in the multithreaded case. With [2], the issue of multiple breakpoints on the same should no longer affect this test. [1]: swiftlang#9839 [2]: swiftlang#9112

With [1], stepping should work reliably even in the multithreaded case. With [2], the issue of multiple breakpoints on the same should no longer affect this test. [1]: #9839 [2]: #9112 (cherry picked from commit 4ab6819)

The initial implementation of swift async stepping involved migrating ThreadPlans from thread to thread. With the OS plugins introduced in [1], this is no longer needed and the associated code is dead. To find the relevant diffs, files were compared with their upstream counterparts, and code mentioning the following concepts was deleted: * "synchronizing plans", * "detached plans", * "setting TIDs on a thread plan", * "activating a thread plan stack". The storage for thread plan stacks had been substantially changed downstream to allow for the concepts above. This patch restores the original storage data structures. These were the files compared: * ThreadPlanStack{.cpp, .h} * ThreadPlan{.cpp, .h} * Process{.cpp, .h} [1]: swiftlang#9839

The initial implementation of swift async stepping involved migrating ThreadPlans from thread to thread. With the OS plugins introduced in [1], this is no longer needed and the associated code is dead. To find the relevant diffs, files were compared with their upstream counterparts, and code mentioning the following concepts was deleted: * "synchronizing plans", * "detached plans", * "setting TIDs on a thread plan", * "activating a thread plan stack". The storage for thread plan stacks had been substantially changed downstream to allow for the concepts above. This patch restores the original storage data structures. These were the files compared: * ThreadPlanStack{.cpp, .h} * ThreadPlan{.cpp, .h} * Process{.cpp, .h} [1]: #9839 (cherry picked from commit c8c7df7)

adrian-prantl reviewed Jan 15, 2025

View reviewed changes

lldb/source/Plugins/OperatingSystem/Swift/OperatingSystemSwift.cpp Outdated Show resolved Hide resolved

adrian-prantl reviewed Jan 15, 2025

View reviewed changes

lldb/source/Plugins/OperatingSystem/Swift/OperatingSystemSwift.cpp Outdated Show resolved Hide resolved

adrian-prantl reviewed Jan 15, 2025

View reviewed changes

lldb/source/Plugins/OperatingSystem/Swift/OperatingSystemSwift.cpp Outdated Show resolved Hide resolved

felipepiovezan commented Jan 15, 2025

View reviewed changes

lldb/source/Plugins/OperatingSystem/Swift/OperatingSystemSwift.cpp Outdated Show resolved Hide resolved

jimingham reviewed Jan 15, 2025

View reviewed changes

jimingham reviewed Jan 16, 2025

View reviewed changes

felipepiovezan force-pushed the felipe/swift_os_plugin_rebase_61 branch 3 times, most recently from 969ca8a to 8aeb614 Compare January 24, 2025 02:46

felipepiovezan commented Jan 24, 2025

View reviewed changes

felipepiovezan force-pushed the felipe/swift_os_plugin_rebase_61 branch from 8aeb614 to 9bd7fa3 Compare January 24, 2025 03:06

felipepiovezan changed the base branch from swift/release/6.1 to stable/20240723 January 24, 2025 17:15

kastiglione mentioned this pull request Jan 29, 2025

[lldb] Introduce Swift "task info" command #9875

Merged

felipepiovezan force-pushed the felipe/swift_os_plugin_rebase_61 branch from 9bd7fa3 to 5142ea2 Compare February 4, 2025 15:20

medismailben reviewed Feb 4, 2025

View reviewed changes

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp Show resolved Hide resolved

medismailben reviewed Feb 4, 2025

View reviewed changes

felipepiovezan force-pushed the felipe/swift_os_plugin_rebase_61 branch from 5142ea2 to cdc4b54 Compare February 4, 2025 21:52

felipepiovezan marked this pull request as ready for review February 4, 2025 21:52

felipepiovezan requested a review from a team as a code owner February 4, 2025 21:52

kastiglione reviewed Feb 4, 2025

View reviewed changes

jimingham reviewed Feb 5, 2025

View reviewed changes

felipepiovezan force-pushed the felipe/swift_os_plugin_rebase_61 branch 2 times, most recently from f1a3807 to bf4eb62 Compare February 6, 2025 22:23

felipepiovezan commented Feb 6, 2025

View reviewed changes

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp Show resolved Hide resolved

felipepiovezan force-pushed the felipe/swift_os_plugin_rebase_61 branch from bf4eb62 to 4de0f0c Compare February 6, 2025 22:34

jimingham approved these changes Feb 6, 2025

View reviewed changes

medismailben approved these changes Feb 6, 2025

View reviewed changes

felipepiovezan force-pushed the felipe/swift_os_plugin_rebase_61 branch from 4de0f0c to 33da3aa Compare February 7, 2025 17:27

felipepiovezan requested a review from a team as a code owner February 7, 2025 17:27

felipepiovezan added 3 commits February 10, 2025 10:00

[lldb][swift] Add helper function to find Swift concurrency's module …

766ba98

…version

[lldb] Add experimental target option controlling swift OS plugin

62b2f52

This is here mostly out of an abundance of caution, the default is to use the plugins.

felipepiovezan force-pushed the felipe/swift_os_plugin_rebase_61 branch from 33da3aa to dd46e67 Compare February 10, 2025 18:00

adrian-prantl approved these changes Feb 11, 2025

View reviewed changes

felipepiovezan merged commit 9c662b1 into swiftlang:stable/20240723 Feb 11, 2025
3 checks passed

felipepiovezan deleted the felipe/swift_os_plugin_rebase_61 branch February 11, 2025 00:41

felipepiovezan mentioned this pull request Feb 11, 2025

[lldb] Re-enable swift step-in test #9999

Merged

This was referenced Feb 11, 2025

[lldb][NFC] Remove vestigial downstream changes related to ThreadPlan migration #10000

Merged

[cherry-pick][lldb][swift] Swift OS Plugin #10001

Merged

[lldb] Re-enable swift step-in test #10006

Merged

felipepiovezan mentioned this pull request Feb 20, 2025

[lldb][NFC] Remove downstream changes related to ThreadPlan migration #10077

Merged

	ModuleSP SwiftLanguageRuntime::findConcurrencyModule(Process &process) {
	ModuleSP SwiftLanguageRuntime::FindConcurrencyModule(Process &process) {

		if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);
		IsOperatingSystemPluginThread(old_thread))

	if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);
	IsOperatingSystemPluginThread(old_thread))
	if ((ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id)) && IsOperatingSystemPluginThread(old_thread))

[lldb] Swift OS plugin #9839

[lldb] Swift OS plugin #9839

Uh oh!

Conversation

felipepiovezan commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimingham commented Jan 16, 2025

Uh oh!

felipepiovezan Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

medismailben left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipepiovezan Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipepiovezan commented Feb 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipepiovezan commented Jan 15, 2025 •

edited

Loading

felipepiovezan Jan 24, 2025 •

edited

Loading

felipepiovezan Feb 4, 2025 •

edited

Loading

felipepiovezan Feb 6, 2025 •

edited

Loading

felipepiovezan Feb 6, 2025 •

edited

Loading