-
Notifications
You must be signed in to change notification settings - Fork 340
[lldb] Fix stepping into Objective-C interop ctors #10697
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
base: swift/release/6.2
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -45,6 +45,7 @@ enum class ThunkKind { | |||||
AllocatingInit, | ||||||
PartialApply, | ||||||
ObjCAttribute, | ||||||
NonObjCAttributeOnCtor, | ||||||
Reabstraction, | ||||||
ProtocolConformance, | ||||||
}; | ||||||
|
@@ -55,6 +56,7 @@ enum class ThunkAction { | |||||
StepIntoConformance, | ||||||
StepIntoAllocatingInit, | ||||||
StepThrough, | ||||||
RunToObjcCInteropCtor, | ||||||
}; | ||||||
|
||||||
} // namespace | ||||||
|
@@ -313,6 +315,10 @@ static ThunkKind GetThunkKind(Symbol *symbol) { | |||||
switch (main_node->getKind()) { | ||||||
case Node::Kind::ObjCAttribute: | ||||||
return ThunkKind::ObjCAttribute; | ||||||
case Node::Kind::NonObjCAttribute: | ||||||
if (hasChild(nodes, Node::Kind::Constructor)) | ||||||
return ThunkKind::NonObjCAttributeOnCtor; | ||||||
break; | ||||||
case Node::Kind::ProtocolWitness: | ||||||
if (hasChild(main_node, Node::Kind::ProtocolConformance)) | ||||||
return ThunkKind::ProtocolConformance; | ||||||
|
@@ -342,6 +348,8 @@ static const char *GetThunkKindName(ThunkKind kind) { | |||||
return "GetThunkTarget"; | ||||||
case ThunkKind::ObjCAttribute: | ||||||
return "GetThunkTarget"; | ||||||
case ThunkKind::NonObjCAttributeOnCtor: | ||||||
return "RunToObjcCInteropCtor"; | ||||||
case ThunkKind::Reabstraction: | ||||||
return "GetThunkTarget"; | ||||||
case ThunkKind::ProtocolConformance: | ||||||
|
@@ -363,6 +371,8 @@ static ThunkAction GetThunkAction(ThunkKind kind) { | |||||
return ThunkAction::StepThrough; | ||||||
case ThunkKind::ProtocolConformance: | ||||||
return ThunkAction::StepIntoConformance; | ||||||
case ThunkKind::NonObjCAttributeOnCtor: | ||||||
return ThunkAction::RunToObjcCInteropCtor; | ||||||
} | ||||||
} | ||||||
|
||||||
|
@@ -419,6 +429,66 @@ CreateRunThroughTaskSwitchingTrampolines(Thread &thread, | |||||
return nullptr; | ||||||
} | ||||||
|
||||||
// Search all modules for `target_func` and creates a RunToAddress plan if a | ||||||
// single function is found. | ||||||
static ThreadPlanSP CreateRunToAddressPlan(StringRef target_func, | ||||||
Thread &thread, bool stop_others) { | ||||||
ModuleList modules = thread.GetProcess()->GetTarget().GetImages(); | ||||||
SymbolContextList sc_list; | ||||||
modules.FindFunctionSymbols(ConstString(target_func), eFunctionNameTypeFull, | ||||||
sc_list); | ||||||
if (sc_list.GetSize() != 1 || sc_list[0].symbol == nullptr) | ||||||
return nullptr; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit confused by this suggestion, the function returns a pointer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
Symbol &thunk_symbol = *sc_list[0].symbol; | ||||||
Address target_address = thunk_symbol.GetAddress(); | ||||||
if (target_address.IsValid()) | ||||||
return std::make_shared<ThreadPlanRunToAddress>(thread, target_address, | ||||||
stop_others); | ||||||
return nullptr; | ||||||
} | ||||||
|
||||||
/// Demangle `symbol_name` and extracts the text at the node described by | ||||||
/// `node_path`, if it exists. | ||||||
static std::optional<std::string> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any value in the optional or can you just use the empty string as null value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can change this to implicitly use the empty string as a "failed" value There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Swift doesn't have any "anonymous" entities like C & C++ do? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hopefully nobody has anonymous classes! |
||||||
FindClassName(StringRef symbol_name, llvm::ArrayRef<Node::Kind> node_path) { | ||||||
swift::Demangle::Context ctx; | ||||||
NodePointer demangled_node = | ||||||
SwiftLanguageRuntime::DemangleSymbolAsNode(symbol_name, ctx); | ||||||
|
||||||
NodePointer class_node = childAtPath(demangled_node, node_path); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if childAtPath handles null NodePointer arguments, still seem easier to follow if you checked the return here. |
||||||
if (!class_node || !class_node->hasText()) { | ||||||
std::string node_str = getNodeTreeAsString(demangled_node); | ||||||
LLDB_LOGF(GetLog(LLDBLog::Step), | ||||||
"SwiftLanguageRuntime: failed to extract name from " | ||||||
"demangle node: %s", | ||||||
node_str.c_str()); | ||||||
return {}; | ||||||
} | ||||||
return class_node->getText().str(); | ||||||
} | ||||||
|
||||||
/// If sc_list is non-empty, returns a plan that runs to any of its addresses. | ||||||
/// Otherwise, returns nullptr. | ||||||
static ThreadPlanSP CreateThreadPlanRunToAnySc(Thread &thread, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd abbreviate SymbolContext to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, this is a generally useful, not swift specific bit of functionality, so it's wrong to have it in a Swift-specific file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RunToAnySc is also a really ambitious function name. Maybe RunToSCInList? |
||||||
SymbolContextList &sc_list, | ||||||
bool stop_others) { | ||||||
std::vector<addr_t> load_addresses; | ||||||
Target &target = thread.GetProcess()->GetTarget(); | ||||||
for (const SymbolContext &ctor_sc : sc_list) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't call this |
||||||
const Symbol *ctor_symbol = ctor_sc.symbol; | ||||||
if (ctor_symbol) | ||||||
load_addresses.push_back(ctor_symbol->GetLoadAddress(&target)); | ||||||
} | ||||||
|
||||||
if (load_addresses.empty()) { | ||||||
LLDB_LOG(GetLog(LLDBLog::Step), | ||||||
"SwiftLanguageRuntime: empty sc_list found."); | ||||||
return nullptr; | ||||||
} | ||||||
return std::make_shared<ThreadPlanRunToAddress>(thread, load_addresses, | ||||||
stop_others); | ||||||
} | ||||||
|
||||||
static lldb::ThreadPlanSP GetStepThroughTrampolinePlan(Thread &thread, | ||||||
bool stop_others) { | ||||||
// Here are the trampolines we have at present. | ||||||
|
@@ -475,19 +545,26 @@ static lldb::ThreadPlanSP GetStepThroughTrampolinePlan(Thread &thread, | |||||
log->Printf( | ||||||
"Stepped to thunk \"%s\" (kind: %s) stepping to target: \"%s\".", | ||||||
symbol_name, GetThunkKindName(thunk_kind), thunk_target.c_str()); | ||||||
return CreateRunToAddressPlan(thunk_target, thread, stop_others); | ||||||
} | ||||||
case ThunkAction::RunToObjcCInteropCtor: { | ||||||
LLDB_LOG(log, "SwiftLanguageRuntime: running to " | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
"objective C constructor from swift."); | ||||||
static constexpr auto class_path = { | ||||||
Node::Kind::Constructor, Node::Kind::Class, Node::Kind::Identifier}; | ||||||
std::optional<std::string> class_name = FindClassName(symbol_name, class_path); | ||||||
if (!class_name) | ||||||
return nullptr; | ||||||
std::string ctor_name = llvm::formatv("{0} init", *class_name); | ||||||
|
||||||
ModuleList modules = thread.GetProcess()->GetTarget().GetImages(); | ||||||
SymbolContextList sc_list; | ||||||
modules.FindFunctionSymbols(ConstString(thunk_target), | ||||||
eFunctionNameTypeFull, sc_list); | ||||||
if (sc_list.GetSize() == 1 && sc_list[0].symbol) { | ||||||
Symbol &thunk_symbol = *sc_list[0].symbol; | ||||||
Address target_address = thunk_symbol.GetAddress(); | ||||||
if (target_address.IsValid()) | ||||||
return std::make_shared<ThreadPlanRunToAddress>(thread, target_address, | ||||||
stop_others); | ||||||
} | ||||||
return nullptr; | ||||||
ModuleFunctionSearchOptions options{/*include_symbols*/ true, | ||||||
/*include_inlines*/ true}; | ||||||
ModuleList modules = thread.GetProcess()->GetTarget().GetImages(); | ||||||
modules.FindFunctions(RegularExpression(ctor_name), options, sc_list); | ||||||
|
||||||
ThreadPlanSP plan = CreateThreadPlanRunToAnySc(thread, sc_list, stop_others); | ||||||
return plan; | ||||||
} | ||||||
case ThunkAction::StepIntoConformance: { | ||||||
// The TTW symbols encode the protocol conformance requirements | ||||||
|
@@ -582,37 +659,20 @@ static lldb::ThreadPlanSP GetStepThroughTrampolinePlan(Thread &thread, | |||||
} | ||||||
case ThunkAction::StepIntoAllocatingInit: { | ||||||
LLDB_LOGF(log, "Stepping into allocating init: \"%s\"", symbol_name); | ||||||
swift::Demangle::Context ctx; | ||||||
NodePointer demangled_node = | ||||||
SwiftLanguageRuntime::DemangleSymbolAsNode(symbol_name, ctx); | ||||||
|
||||||
using Kind = Node::Kind; | ||||||
NodePointer class_node = childAtPath( | ||||||
demangled_node, {Kind::Allocator, Kind::Class, Kind::Identifier}); | ||||||
if (!class_node || !class_node->hasText()) { | ||||||
std::string node_str = getNodeTreeAsString(demangled_node); | ||||||
LLDB_LOGF(log, | ||||||
"Failed to extract constructor name from demangle node: %s", | ||||||
node_str.c_str()); | ||||||
static constexpr auto class_path = {Kind::Allocator, Kind::Class, | ||||||
Kind::Identifier}; | ||||||
std::optional<std::string> class_name = | ||||||
FindClassName(symbol_name, class_path); | ||||||
if (!class_name) | ||||||
return nullptr; | ||||||
} | ||||||
|
||||||
ModuleFunctionSearchOptions options{/*include_symbols*/ true, | ||||||
/*include_inlines*/ true}; | ||||||
std::string ctor_name = llvm::formatv("{0}.init", class_node->getText()); | ||||||
std::string ctor_name = llvm::formatv("{0}.init", *class_name); | ||||||
SymbolContextList sc_list; | ||||||
sc.module_sp->FindFunctions(RegularExpression(ctor_name), options, sc_list); | ||||||
std::vector<addr_t> load_addresses; | ||||||
Target &target = thread.GetProcess()->GetTarget(); | ||||||
for (const SymbolContext &ctor_sc : sc_list) { | ||||||
const Symbol *ctor_symbol = ctor_sc.symbol; | ||||||
if (ctor_symbol) | ||||||
load_addresses.push_back(ctor_symbol->GetLoadAddress(&target)); | ||||||
} | ||||||
if (load_addresses.empty()) | ||||||
return nullptr; | ||||||
return std::make_shared<ThreadPlanRunToAddress>(thread, load_addresses, | ||||||
stop_others); | ||||||
return CreateThreadPlanRunToAnySc(thread, sc_list, stop_others); | ||||||
} | ||||||
case ThunkAction::StepThrough: { | ||||||
if (log) | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
#import <Foundation/Foundation.h> | ||
|
||
@interface Foo : NSObject | ||
|
||
@property (nonnull) NSArray<NSString *> *values; | ||
|
||
- (nonnull id)init; | ||
- (nonnull id)initWithString:(nonnull NSString *)value; | ||
- (nonnull id)initWithString:(nonnull NSString *)value andOtherString:(nonnull NSString *) otherValue; | ||
|
||
@end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
#import "Foo.h" | ||
|
||
@implementation Foo | ||
|
||
- (id)init { | ||
} | ||
|
||
- (id)initWithString:(nonnull NSString *)value { | ||
self->_values = @[value]; | ||
return self; | ||
} | ||
|
||
- (nonnull id)initWithString:(nonnull NSString *)value andOtherString:(nonnull NSString *) otherValue { | ||
self->_values = @[value, otherValue]; | ||
return self; | ||
} | ||
|
||
@end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
SWIFT_SOURCES := main.swift | ||
SWIFT_BRIDGING_HEADER := bridging-header.h | ||
OBJC_SOURCES := Foo.m | ||
SWIFT_OBJC_INTEROP := 1 | ||
|
||
include Makefile.rules |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
import lldb | ||
from lldbsuite.test.lldbtest import * | ||
from lldbsuite.test.decorators import * | ||
import lldbsuite.test.lldbutil as lldbutil | ||
|
||
|
||
class TestSwiftObjcProtocol(TestBase): | ||
@skipUnlessDarwin | ||
@swiftTest | ||
def test(self): | ||
self.build() | ||
(target, process, thread, breakpoint) = lldbutil.run_to_source_breakpoint( | ||
self, "break here", lldb.SBFileSpec("main.swift") | ||
) | ||
|
||
# Go to the first constructor, assert we can step into it. | ||
thread.StepInto() | ||
self.assertEqual(thread.stop_reason, lldb.eStopReasonPlanComplete) | ||
self.assertIn("-[Foo init]", thread.frames[0].GetFunctionName()) | ||
|
||
# Go back to "work" function | ||
thread.StepOut() | ||
self.assertEqual(thread.stop_reason, lldb.eStopReasonPlanComplete) | ||
self.assertIn("work", thread.frames[0].GetFunctionName()) | ||
|
||
# Go to the next constructor call. | ||
thread.StepOver() | ||
self.assertEqual(thread.stop_reason, lldb.eStopReasonPlanComplete) | ||
self.assertIn("work", thread.frames[0].GetFunctionName()) | ||
|
||
# Assert we can step into it. | ||
thread.StepInto() | ||
self.assertEqual(thread.stop_reason, lldb.eStopReasonPlanComplete) | ||
self.assertIn("-[Foo initWithString:]", thread.frames[0].GetFunctionName()) | ||
|
||
# Go back to "work" function | ||
thread.StepOut() | ||
self.assertEqual(thread.stop_reason, lldb.eStopReasonPlanComplete) | ||
self.assertIn("work", thread.frames[0].GetFunctionName()) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
#import "Foo.h" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
func work() { | ||
let noParams = Foo() // break here | ||
let oneParam = Foo(string: "Bar") | ||
print("done") | ||
} | ||
|
||
work() |
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 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.