This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Make Target* a Target& in CommandObjectExpression::DoExecute REPL logic
ClosedPublic

Authored by teemperor on Nov 8 2019, 3:28 AM.

Diff Detail

Event Timeline

teemperor created this revision.Nov 8 2019, 3:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2019, 3:28 AM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 8 2019, 3:29 AM
This revision was automatically updated to reflect the committed changes.

As long as target.GetREPL (..., true) returns a sensible error for the Dummy target, this is fine. Actually having the dummy target run the REPL seems like a bad idea, since that's a pretty heavy-weight activity and the dummy target is supposed to be just for priming settings for future targets.

I don't think either version is right.

We really shouldn't be using "GetSelectedTarget" (with or without the Dummy) for getting the Target context for a command. That's how it was originally done, but you aren't guaranteed that the selected target is the right target context in callbacks that use commands.

For instance, if lldb has two targets running in one debugger, and Target A is the currently selected target, but target B hits a breakpoint and the breakpoint has commands, we need to make sure the commands run in the breakpoint use target B. But we don't select Target B when we hit the breakpoint, since I want to keep that Selected Target more under the user's control. Instead we set the appropriate target in the command's m_exe_ctx, and commands are supposed to pull it from there. BTW, I see that there are still a bunch of commands that haven't been updated to use this. Maybe we ought to have a "GetCurrentOrDummyTarget" in CommandObject that returns the target from m_exe_ctx, or the dummy target if that is empty. Either that or put the Dummy target IN m_exe_ctx if there is no appropriate target, and have everybody use that.

Anyway, this isn't more wrong than the previous version, but it isn't right either...