Page MenuHomePhabricator

[lldb] Inline invariant params to AppleThreadPlanStepThrough (NFC)
ClosedPublic

Authored by kastiglione on Feb 8 2021, 9:23 AM.

Details

Summary

These two AppleThreadPlanStepThrough thread plans have parameterized behavior
that is unutilized. To make their interface and implementation simpler, this
change inlines those outside parameters.

Diff Detail

Event Timeline

kastiglione requested review of this revision.Feb 8 2021, 9:23 AM
kastiglione created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2021, 9:23 AM
kastiglione added inline comments.Feb 8 2021, 9:45 AM
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp
1163

This is one parameter being inlined.

1188–1189

These are two other parameters being inlined.

jingham added a comment.EditedFeb 8 2021, 9:56 AM

I can't think of a really good reason why you would need to override the general "step in avoids nodebug" behavior. I'm pretty sure I was thinking of a trampoline that got you half way to somewhere interesting, at which point you would want to negotiate again for the trampoline to take you the rest of the way. In that case you would not want to obey the general setting. But this setting controls what happens when you don't find a good place to go, so that control isn't necessary.

It still seems to me like a trampoline which knew that to implement itself, all it had to do was a couple of stepi's - for instance if a dyld stub knew that the stub had been filled in and would jump straight to the target, it could run without letting the other threads go. That's actually something the dyld stubs could figure out, they just don't at present. So I'd rather not remove this control.

It still seems to me like a trampoline which knew that to implement itself, all it had to do was a couple of stepi's - for instance if a dyld stub knew that the stub had been filled in and would jump straight to the target, it could run without letting the other threads go. That's actually something the dyld stubs could figure out, they just don't at present. So I'd rather not remove this control.

Happy to restore the stop_others parameter, but for my understanding I have two questions:

  1. Is the behavior you envision something that applies to these specific thread plan subclasses (ie not dyld stubs)
  2. Will callers decide to stop others from the outside, or is the decision to stop others something that these subclasses would make internally?

It still seems to me like a trampoline which knew that to implement itself, all it had to do was a couple of stepi's - for instance if a dyld stub knew that the stub had been filled in and would jump straight to the target, it could run without letting the other threads go. That's actually something the dyld stubs could figure out, they just don't at present. So I'd rather not remove this control.

Happy to restore the stop_others parameter, but for my understanding I have two questions:

  1. Is the behavior you envision something that applies to these specific thread plan subclasses (ie not dyld stubs)
  2. Will callers decide to stop others from the outside, or is the decision to stop others something that these subclasses would make internally?

Good questions. I was reading this incorrectly. This is the enqueuing plan telling the StepThrough plan whether to stop others. That actually seems like a bad thing to do, since the enqueuer can't really know whether it is actually safe to stop others while running the enqueued plan. These should go.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 9 2021, 8:04 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.