[lldb] Fix stepping into Objective-C interop ctors #10697

felipepiovezan · 2025-05-16T22:18:00Z

The first commit is just hoisting helper functions for re-use. The second commit actually solves the problem.

Please read each commit in isolation, especially the message on the second commit.

rdar://146886271

felipepiovezan · 2025-05-16T22:24:22Z

adrian-prantl · 2025-05-16T22:35:02Z

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

+  modules.FindFunctionSymbols(ConstString(target_func), eFunctionNameTypeFull,
+                              sc_list);
+  if (sc_list.GetSize() != 1 || sc_list[0].symbol == nullptr)
+    return nullptr;


Suggested change

return nullptr;

return {};

I'm a bit confused by this suggestion, the function returns a pointer

It returns a ThreadPlanSP (the success return returns make_shared). Apparently C++ is smart enough to turn nullptr into a default constructed shared pointer, but I agree with Adrian, that's a little magical for my taste.

Interesting, I will change, but note my dissent here in case it persuades you.

A reader looking at {} will not get any of the mental warnings a nullptr brings.

Share pointers are, for all intents and purposes, pointers. Therefore there is nothing magical about the nullptr constructor: it's just a pointer object being constructed from a pointer value, akin to returning nullopt in a function producing std::optional<T>.

Reading return {} is more likely to make the reader think: "wait, what is the return type of this function again?" (plus it requires the reader knowing what a default-constructed ThreadPlanSP does) whereas reading return nullptr is explicit.

The final reason to keep it as nullptr is that all other places in this file are doing that, so this would stand out.

If either of those convince you, I'll happily switch back to nullptr :)

I didn't mean to write an essay, but I do believe this is the wrong direction, so it is worth talking through the decision.

adrian-prantl · 2025-05-16T22:35:39Z

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

+
+/// Demangle `symbol_name` and extracts the text at the node described by
+/// `node_path`, if it exists.
+static std::optional<std::string>


Is there any value in the optional or can you just use the empty string as null value?

We can change this to implicitly use the empty string as a "failed" value

Swift doesn't have any "anonymous" entities like C & C++ do?

Hopefully nobody has anonymous classes!

felipepiovezan · 2025-05-17T01:31:56Z

From the test failure:

16:53:31      self.assertIn("-[Foo initWithString:]", thread.frames[0].GetFunctionName())
16:53:31  AssertionError: '-[Foo initWithString:]' not found in 'generic specialization <serialized, Swift.UInt8> of Swift.UnsafeBufferPointer.init(start: Swift.Optional<Swift.UnsafePointer<τ_0_0>>, count: Swift.Int) -> Swift.UnsafeBufferPointer<τ_0_0>'

Standard library with debug info strikes again.

jimingham · 2025-05-20T22:35:03Z

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

+  SymbolContextList sc_list;
+  modules.FindFunctionSymbols(ConstString(target_func), eFunctionNameTypeFull,
+                              sc_list);
+  if (sc_list.GetSize() != 1 || sc_list[0].symbol == nullptr)


Why do we care that there's only one? For instance, if this is an ObjC method, two shared libraries can have implementations of the same ObjC class. The runtime will pick which one to use, but we don't actually know from symbols which one that is.
And since this is a thread specific breakpoint, the only way setting a breakpoint on the "wrong" function as well as the "right" would cause trouble is if running from the thunk to the target function called the wrong function, which seems unlikely.

Good points, I'll change this. This way we can also delegate part of the functionality to the other function introduced in this commit

jimingham · 2025-05-20T22:38:42Z

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

+  NodePointer demangled_node =
+      SwiftLanguageRuntime::DemangleSymbolAsNode(symbol_name, ctx);
+
+  NodePointer class_node = childAtPath(demangled_node, node_path);


Even if childAtPath handles null NodePointer arguments, still seem easier to follow if you checked the return here.

Will do.

If we're taking this approach, it would be worth being consistent everywhere (in a separate patch) and changing childAtPath to take a reference instead, so that it no longer accepts nullptrs by definition.

While I believe the current approach leads to more succinct code, either one is fine as long as we're consistent. The only undesirable outcome is the middle ground where the callee handles nullptrs but callers are also expected to be checking. Either we trust the API to do things correctly (the API said it would when it declared itself taking a pointer), or we change the API to take a reference. APIs that accept pointers but don't handle null are bad APIs.

Ops, I forgot to do this. Will push another patch shortly

edit: done

jimingham · 2025-05-20T22:39:30Z

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

+
+/// If sc_list is non-empty, returns a plan that runs to any of its addresses.
+/// Otherwise, returns nullptr.
+static ThreadPlanSP CreateThreadPlanRunToAnySc(Thread &thread,


I'd abbreviate SymbolContext to SC not Sc, the latter looks weird.

Also, this is a generally useful, not swift specific bit of functionality, so it's wrong to have it in a Swift-specific file.

RunToAnySc is also a really ambitious function name. Maybe RunToSCInList?

RunToAnySc is also a really ambitious function name. Maybe RunToSCInList?

Good idea!

Also, this is a generally useful, not swift specific bit of functionality, so it's wrong to have it in a Swift-specific file.

I agree we should do that, but I think it is important to take this first step -- where we identified a useful piece of logic in the middle of a big function -- in a self-contained commit. Then we can look upstream, find other places that require this logic, create the same function there, and then cherry-pick it here and delete this code.

jimingham · 2025-05-20T22:40:53Z

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

+                                               bool stop_others) {
+  std::vector<addr_t> load_addresses;
+  Target &target = thread.GetProcess()->GetTarget();
+  for (const SymbolContext &ctor_sc : sc_list) {


I wouldn't call this ctor_sc, nowhere in this function do you assume the symbol is a ctor, so that's just confusing.

Oops, good catch, an artifact of earlier iterations of this patch

jimingham · 2025-05-20T22:44:48Z

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

+    return CreateRunToAddressPlan(thunk_target, thread, stop_others);
+  }
+  case ThunkAction::RunToObjcCInteropCtor: {
+    LLDB_LOG(log, "SwiftLanguageRuntime: running to "


Can you wait the log output till you've got the class name? It seems useful to know what class we thought we were supposed to run to.

You could also then log the case where somebody passes you a demangled name you couldn't find the class it should have targeted, which would also be nice to see.

Great points, I'll improve the logging here

jimingham · 2025-05-20T22:49:10Z

This seems like a fine strategy, I had some nit picks but nothing serious.

felipepiovezan

Addressed all review comments, though I did push back on a couple of them, so please let me know if you have follow-up thoughts. While I pushed back, I did what was asked, but I'm interested in the discussion :)

Also applied the usual fix for standard libraries containing debug info. Separately, we should create a helper function for all tests to use when doing this.

@swift-ci test

felipepiovezan · 2025-05-22T21:17:24Z

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

+  modules.FindFunctionSymbols(ConstString(target_func), eFunctionNameTypeFull,
+                              sc_list);
+  if (sc_list.GetSize() != 1 || sc_list[0].symbol == nullptr)
+    return nullptr;


Interesting, I will change, but note my dissent here in case it persuades you.

A reader looking at {} will not get any of the mental warnings a nullptr brings.

Share pointers are, for all intents and purposes, pointers. Therefore there is nothing magical about the nullptr constructor: it's just a pointer object being constructed from a pointer value, akin to returning nullopt in a function producing std::optional<T>.

Reading return {} is more likely to make the reader think: "wait, what is the return type of this function again?" (plus it requires the reader knowing what a default-constructed ThreadPlanSP does) whereas reading return nullptr is explicit.

The final reason to keep it as nullptr is that all other places in this file are doing that, so this would stand out.

If either of those convince you, I'll happily switch back to nullptr :)

I didn't mean to write an essay, but I do believe this is the wrong direction, so it is worth talking through the decision.

felipepiovezan · 2025-05-22T21:20:08Z

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

+  NodePointer demangled_node =
+      SwiftLanguageRuntime::DemangleSymbolAsNode(symbol_name, ctx);
+
+  NodePointer class_node = childAtPath(demangled_node, node_path);


Will do.

If we're taking this approach, it would be worth being consistent everywhere (in a separate patch) and changing childAtPath to take a reference instead, so that it no longer accepts nullptrs by definition.

While I believe the current approach leads to more succinct code, either one is fine as long as we're consistent. The only undesirable outcome is the middle ground where the callee handles nullptrs but callers are also expected to be checking. Either we trust the API to do things correctly (the API said it would when it declared itself taking a pointer), or we change the API to take a reference. APIs that accept pointers but don't handle null are bad APIs.

felipepiovezan · 2025-05-22T22:02:16Z

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

+
+/// If sc_list is non-empty, returns a plan that runs to any of its addresses.
+/// Otherwise, returns nullptr.
+static ThreadPlanSP CreateThreadPlanRunToAnySc(Thread &thread,


RunToAnySc is also a really ambitious function name. Maybe RunToSCInList?

Good idea!

Also, this is a generally useful, not swift specific bit of functionality, so it's wrong to have it in a Swift-specific file.

I agree we should do that, but I think it is important to take this first step -- where we identified a useful piece of logic in the middle of a big function -- in a self-contained commit. Then we can look upstream, find other places that require this logic, create the same function there, and then cherry-pick it here and delete this code.

felipepiovezan · 2025-05-22T22:02:43Z

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

+                                               bool stop_others) {
+  std::vector<addr_t> load_addresses;
+  Target &target = thread.GetProcess()->GetTarget();
+  for (const SymbolContext &ctor_sc : sc_list) {


Oops, good catch, an artifact of earlier iterations of this patch

felipepiovezan · 2025-05-23T01:31:08Z

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

+  SymbolContextList sc_list;
+  modules.FindFunctionSymbols(ConstString(target_func), eFunctionNameTypeFull,
+                              sc_list);
+  if (sc_list.GetSize() != 1 || sc_list[0].symbol == nullptr)


Good points, I'll change this. This way we can also delegate part of the functionality to the other function introduced in this commit

felipepiovezan · 2025-05-23T17:06:19Z

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

+    return CreateRunToAddressPlan(thunk_target, thread, stop_others);
+  }
+  case ThunkAction::RunToObjcCInteropCtor: {
+    LLDB_LOG(log, "SwiftLanguageRuntime: running to "


Great points, I'll improve the logging here

felipepiovezan · 2025-05-23T21:02:40Z

@swift-ci test

These will be useful to reuse code in upcoming commits.

When constructing an Objective C object of type `Foo` from Swift, this sequence of function calls is used: ``` * frame #0: 0x000000010000147c test.out`-[Foo initWithString:](self=0x00006000023ec000, _cmd="initWithString:", value=@"Bar") -[Foo initWithString:] at Foo.m:9:21 frame swiftlang#1: 0x00000001000012bc test.out`@nonobjc Foo.init(string:) $sSo3FooC6stringABSS_tcfcTO at <compiler-generated>:0 frame swiftlang#2: 0x0000000100001170 test.out`Foo.__allocating_init(string:) $sSo3FooC6stringABSS_tcfC at Foo.h:0 frame swiftlang#3: 0x0000000100000ed8 test.out`work() $s4test4workyyF at main.swift:5:18 ``` Frames 1 and 2 are common with pure Swift classes, and LLDB has a Thread Plan to go from `Foo.allocating_init` -> `Foo.init`. In the case of Objcetive C interop, `Foo.init` has no user code, and is annotated with `@nonobjc`. The debugger needs a plan to go from that code to the Objective C implementation. This is what this patch attempts to fix by creating a plan that runs to any symbol matching `Foo init` (this will match all the :withBlah suffixes). This seems to be the only possible fix for this. While Objective C constructors are not necessarily called init, the interop layer seems to assume this. The only other alternative has some obstacles that could not be easily overcome. Here's the main idea for that. The assembly for `@nonobjc Foo.init` looks like (deleted all non branches): ``` test.out`@nonobjc Foo.init(string:): ... 0x1000012a0 <+20>: bl 0x100001618 ; symbol stub for: Swift.String._bridgeToObjectiveC() -> __C.NSString ... 0x1000012b8 <+44>: bl 0x100001630 ; symbol stub for: objc_msgSend ... 0x1000012e8 <+92>: ret ``` If we had more String arguments, there would be more calls to `_bridgeToObjectiveC`. The call to `objc_msgSend` is the important one, and LLDB knows how to go from that to the target of the message, LLDB has ThreadPlans for that. However, setting a breakpoint on `objc_msgSend` would fail: the calls to `_bridgeToObjectiveC` may also call `objc_msgSend`, so LLDB would end up in the wrong `objc_msgSend`. This is not entirely bad, LLDB would step back to `Foo.init`. Here's the catch: the language runtime refuses to create other plans if PC is not at the start of the function, which makes sense, as it would not be able to distinguish if its job was already done previously or not, unless it had a stateful plan (which it doesn't today).

felipepiovezan · 2025-05-23T23:58:15Z

addressed one piece of feedback that had escaped me.

felipepiovezan · 2025-05-23T23:58:27Z

@swift-ci test

felipepiovezan requested review from adrian-prantl and jimingham May 16, 2025 22:18

felipepiovezan requested a review from a team as a code owner May 16, 2025 22:18

felipepiovezan force-pushed the felipe/wip_step_into_objectivec_interop branch from 75bd162 to 6d84e65 Compare May 16, 2025 22:22

adrian-prantl approved these changes May 16, 2025

View reviewed changes

jimingham reviewed May 20, 2025

View reviewed changes

felipepiovezan force-pushed the felipe/wip_step_into_objectivec_interop branch 2 times, most recently from 43d107a to c19dd8d Compare May 23, 2025 21:00

felipepiovezan commented May 23, 2025

View reviewed changes

felipepiovezan requested a review from jimingham May 23, 2025 21:03

felipepiovezan added 2 commits May 23, 2025 16:57

[lldb][nfc] Create helper functions in SwiftLanguageRuntimeNames

a9a3ebf

These will be useful to reuse code in upcoming commits.

felipepiovezan force-pushed the felipe/wip_step_into_objectivec_interop branch from c19dd8d to 3512b6a Compare May 23, 2025 23:57

[lldb] Fix stepping into Objective-C interop ctors #10697

Are you sure you want to change the base?

[lldb] Fix stepping into Objective-C interop ctors #10697

Uh oh!

Conversation

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

Uh oh!

Uh oh!

felipepiovezan commented May 16, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipepiovezan commented May 17, 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!

felipepiovezan May 23, 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!

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 May 20, 2025

Uh oh!

felipepiovezan left a comment

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 commented May 23, 2025

Uh oh!

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

Uh oh!

Uh oh!

felipepiovezan commented May 16, 2025 •

edited

Loading

felipepiovezan May 23, 2025 •

edited

Loading

felipepiovezan commented May 23, 2025 •

edited

Loading