[lldb] Swift OS plugin #9839

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

No description provided.

adrian-prantl · 2025-01-15T22:01:02Z

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

@@ -0,0 +1,164 @@
+//===-- OperatingSystemSwift.cpp -----------------------------------------===//


Not important. "OperatingSystemSwift" sounds slightly wrong to me. What about "OperatingSystemSwiftTasks" or "OperatingSystemSwiftConcurrency"?

Yeah the name seems misleading the full lldb affordance name is "Operating System Thread Plugin"

the full lldb affordance name is "Operating System Thread Plugin"

While this may be true, it's worth noting that the only other plugin also uses the short version: "OperatingSystemX", where X = Python.

OperatingSystemSwiftTasks

I'm going to rename to this!

adrian-prantl · 2025-01-15T22:02:42Z

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

+    return nullptr;
+
+  /// Heuristic to only load the plugin for swift programs.
+  FileSpec swift_module_name = FileSpec("libswiftCore.dylib");


This only works in a very narrow subset of Swift programs. You should defer to SwiftLanguageRuntime's helper code.
The cases this doesn't support are: non-Darwin (".dylib"), statically linked Swift stdlib, embedded Swift (no dylibs at all).

Do you have a helper function mind? The only thing I found is GetLikelySwiftImageNamesForModule, which has some very weird behavior I'm not sure I understand.

You could also arrange that this can only be accessed by the SwiftLanguageRuntime, and then its loading will be gated on the Runtime loading, which already has to do this sort of computation.

@jimingham I looked into that option, but AFAICT the SwiftLanguageRuntime is loaded regardless of modules.
I.e. the first call to "load runtimes" will always load it, even if the relevant libraries haven't been loaded yet

@adrian-prantl I don't think the code in SwiftLanguageRuntime handles the cases you mentioned either, it always returns a dylib for Darwin platforms:

static ConstString GetStandardLibraryName(Process &process) { // This result needs to be stored in the constructor. PlatformSP platform_sp(process.GetTarget().GetPlatform()); if (platform_sp) return platform_sp->GetFullNameForDylib( ConstString(SwiftLanguageRuntime::GetStandardLibraryBaseName())); return {}; } ConstString SwiftLanguageRuntime::GetStandardLibraryName() { return ::GetStandardLibraryName(*m_process); } ConstString PlatformDarwin::GetFullNameForDylib(ConstString basename) { if (basename.IsEmpty()) return basename; StreamString stream; stream.Printf("lib%s.dylib", basename.GetCString()); return ConstString(stream.GetString()); }

I found a private method in there that maybe we could expose...

adrian-prantl · 2025-01-15T22:04:33Z

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

+      swift::tls_get_key(swift::tls_key::concurrency_task) * ptr_size;
+  // 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;


You should check that the swift concurrency version is still 1 here.

See: swiftlang/swift#76437

felipepiovezan · 2025-01-15T22:08:12Z

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;


Btw it is probably fine to not do this, TIDs are generally higher numbers and there might be value in exposing the task id directly. If we do mask, we should use some other mask

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 ?

…version

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

medismailben · 2025-02-04T17:46:17Z

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

@@ -91,6 +91,10 @@ const char *SwiftLanguageRuntime::GetStandardLibraryBaseName() {
  return "swiftCore";
 }

+const char *SwiftLanguageRuntime::GetConcurrencyLibraryBaseName() {


nit: Can this return a StringRef or StringLiteral ?

medismailben

cool stuff! I left some comments / suggestions :)

medismailben · 2025-02-04T17:48:29Z

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

@@ -131,6 +149,11 @@ static bool IsStaticSwiftRuntime(Module &image) {
  return image.FindFirstSymbolWithNameAndType(swift_reflection_version_sym);
 }

+static bool IsStaticSwiftConcurrency(Module &image) {
+  static const ConstString task_switch_symbol("_swift_task_switch");


This could be:

Suggested change

static const ConstString task_switch_symbol("_swift_task_switch");

static constexpr llvm::StringLiteral task_switch_symbol = "_swift_task_switch";

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) {


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

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

adrian-prantl reviewed Jan 15, 2025

View reviewed changes

felipepiovezan commented Jan 15, 2025

View reviewed changes

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 added 3 commits February 4, 2025 07:19

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

9cfe06f

…version

[lldb] Add experimental target option controlling swift OS plugin

fe71474

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

[lldb][swift] Swift OS Plugin

5142ea2

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb] Swift OS plugin #9839

[lldb] Swift OS plugin #9839

felipepiovezan commented Jan 15, 2025

adrian-prantl Jan 15, 2025

medismailben Jan 15, 2025

felipepiovezan Jan 21, 2025

adrian-prantl Jan 15, 2025

felipepiovezan Jan 15, 2025

jimingham Jan 15, 2025

felipepiovezan Jan 15, 2025 •

edited

Loading

felipepiovezan Jan 21, 2025

felipepiovezan Jan 21, 2025

adrian-prantl Jan 15, 2025

felipepiovezan Jan 15, 2025

jimingham Jan 15, 2025

felipepiovezan Jan 15, 2025

jimingham Jan 16, 2025

felipepiovezan Jan 21, 2025

jimingham commented Jan 16, 2025

felipepiovezan Jan 24, 2025 •

edited

Loading

medismailben Feb 4, 2025

medismailben Feb 4, 2025

medismailben left a comment

medismailben Feb 4, 2025

medismailben Feb 4, 2025

medismailben Feb 4, 2025

medismailben Feb 4, 2025

medismailben Feb 4, 2025

medismailben Feb 4, 2025

medismailben Feb 4, 2025

medismailben Feb 4, 2025

		@@ -0,0 +1,164 @@
		//===-- OperatingSystemSwift.cpp -----------------------------------------===//

	static const ConstString task_switch_symbol("_swift_task_switch");
	static constexpr llvm::StringLiteral task_switch_symbol = "_swift_task_switch";

	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

Are you sure you want to change the base?

[lldb] Swift OS plugin #9839

Conversation

felipepiovezan commented Jan 15, 2025

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

Choose a reason for hiding this comment

felipepiovezan Jan 15, 2025 • edited Loading

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

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

jimingham commented Jan 16, 2025

felipepiovezan Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medismailben left a 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

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 Jan 15, 2025 •

edited

Loading

felipepiovezan Jan 24, 2025 •

edited

Loading