This is an archive of the discontinued LLVM Phabricator instance.

Handle the new objc direct dispatch accelerator functions for uncommonly overridden methods
ClosedPublic

Authored by jingham on Jan 22 2020, 1:30 PM.

Details

Summary

Clang added a new feature to the ObjC compiler that will translate method
calls to commonly un-overridden methods into a function that checks whether
the method is overridden anywhere and if not directly dispatches to the
NSObject implementation.

That means if you do override any of these methods, "step-in" will not step
into your code, since we hit the wrapper function, which has no debug info,
and immediately step out again.

Add code to recognize these functions as "trampolines" and a thread plan that
will get us from the function to the user code, if overridden.

Note, this is not the same feature as the objc_direct attribute, whereby an ObjC developer can state that a certain method will not be overridden so that the compiler will issue a direct call to the implementation function. That actually shouldn't need debugger support for stepping, since we will step directly into the implementation function, not into some ObjC runtime shim. So we'll either stop or not correctly based on whether the implementation function has debug info or not.

Diff Detail

Event Timeline

jingham created this revision.Jan 22 2020, 1:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2020, 1:30 PM
JDevlieghere added inline comments.Jan 22 2020, 1:57 PM
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
786

You could simplify this by using .emplace(sym_addr, i).

889

If you rewrite this you can get rid of the return_ptr temporary.

if (pos != m_msgSend_map.end()) {
    return &g_dispatch_functions[(*pos).second];
}
return nullptr;
918

Can you initialize this on line 910?

1200

Same here, why not do the assignment when you declare the variables?

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp
202

/// Objective-C ...

317
StopReason stop_reason = stop_info_sp ? stop_info_sp->GetStopReason() : eStopReasonNone;
320

I'd invert the condition and make this an early return with the comment below.

358

Spurious empty line

364

Looks like step_out_should_stop isn't used anywhere else?

if(ThreadPlanStepOut::ShouldStop(event_ptr)) {
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h
67

These should be Doxygen comments before the variable if you're already touching them...

/// This is the function call plan.  We fill it at the start, then ...
lldb::ThreadPlanSP m_func_sp;
77

Can you please clang-format the patch, this should fit on one line.

93

///

110

Let's initialize this in the ctor as well, the inconsistency with the next line leaves me wondering if this is a bug.

jingham updated this revision to Diff 239722.Jan 22 2020, 3:35 PM

Addressed Jonas' review comments.

jingham marked 13 inline comments as done.Jan 22 2020, 3:43 PM

Thanks for the suggestions! A couple were not to my taste, but I fixed all the others.

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp
317

I don't like trigraphs like this. The don't make things clearer to my eye and if you ever wanted to stop on one or the other branches, you are just sad.

320

The logic in this computation is generally goes "if A do X, else if B do Y"... So I don't like to write these:

if (!The One Case I've Had To Handle So Far)

return;

DoStuff();

It isn't really that much clearer, and it means you have to redo it to handle another case.

So I'm happy with it the way it is.

364

I don't see the benefit. What's there is easy to read, and more self-documenting. Having important bits of code in an if statement always makes them harder to read for me. I'm happy the way this is.

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.h
93

This wasn't doc, it was a reminder to me, but it wasn't necessary so I removed it...

JDevlieghere added inline comments.Jan 22 2020, 6:27 PM
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleThreadPlanStepThroughObjCTrampoline.cpp
317

What about

StopReason stop_reason = eStopReasonNone;
 if (stop_info_sp)
   stop_reason = stop_info_sp->GetStopReason();
364

Can we make it at least const then? :-)

jingham updated this revision to Diff 239981.Jan 23 2020, 12:38 PM

Addresses the rest of Jonas' comments.

jingham marked 2 inline comments as done.Jan 23 2020, 12:40 PM

Addressed remaining review comments.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 23 2020, 12:46 PM
This revision was automatically updated to reflect the committed changes.
friss added inline comments.Jan 23 2020, 12:55 PM
lldb/include/lldb/Target/ThreadPlan.h
464–469

The comment is somewhat ambiguous about whether the pushed plan is private or not. The first sentence says yes and the second no. Given the code seems to say no, I suppose the first sentence is wrong?