This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Unify target checking in CommandObject
ClosedPublic

Authored by teemperor on Aug 28 2019, 1:49 AM.

Details

Summary

We currently have several CommandObjects that manually reimplement the checking for a selected target
or a target in the execution context (which is the selected target when they are invoked). This patch removes
all these checks and replaces them by setting the eCommandRequiresTarget flag that Pavel suggested. With
this flag we are doing the same check but without having to duplicate this code in all these CommandObjects.

I also added a GetSelectedTarget() variant of the GetSelectedOrDummyTarget() function to the
CommandObject that checks that the flag is set and then returns a reference to the target. I didn't rewrite
all the target variables from Target * to Target & in this patch as last time this change caused a lot of merge
conflicts in Swift and I would prefer having that in a separate NFC commit.

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.Aug 28 2019, 1:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2019, 1:49 AM

I support anything that reduces the code path differences between user-entered commands and their SBAPI counterparts. Thanks for doing this!

The whole command flags was a late addition, but we were using it for new commands and "when you touch it" kind of changes. That seems to have stalled the conversion, so thanks for completing this!

As a minor style thing, prior to this change CommandObjectThreadStepUntil required a thread, but didn't require a process explicitly, because you can't have a thread without a process. You also can't have a thread without a target since you can't have a process without a target. But your change means that if I specify that I require a thread I now ALSO have to require a target explicitly or I will get an assert calling GetSelectedTarget. That seems a little awkward to me (especially since you still don't have to require a process...) It would be better if eCommandRequiresThread implied eCommandRequiresProcess & eCommandRequiresTarget. That would keep these definitions less noisy.

labath accepted this revision.Aug 28 2019, 12:19 PM

Awesome.

As a minor style thing, prior to this change CommandObjectThreadStepUntil required a thread, but didn't require a process explicitly, because you can't have a thread without a process. You also can't have a thread without a target since you can't have a process without a target. But your change means that if I specify that I require a thread I now ALSO have to require a target explicitly or I will get an assert calling GetSelectedTarget. That seems a little awkward to me (especially since you still don't have to require a process...) It would be better if eCommandRequiresThread implied eCommandRequiresProcess & eCommandRequiresTarget. That would keep these definitions less noisy.

This sounds like a good idea to me.

lldb/source/Commands/CommandObjectTarget.cpp
2615–2617 ↗(On Diff #217580)

Another quirk of clang-format is that it will not re-merge strings that it has split previously. If you manually join these three strings into one, and then re-run clang-format, you'll probably end up with something slightly nicer...

This revision is now accepted and ready to land.Aug 28 2019, 12:19 PM
teemperor updated this revision to Diff 218084.Aug 30 2019, 6:32 AM
  • Removed target requirement where we already have another flag that's implying that we need a target.
  • Properly formatted some strings.
  • Extended check in GetSelectedTarget to also check for flags that imply eCommandRequiresTarget.
clayborg requested changes to this revision.Aug 30 2019, 8:51 AM
clayborg added a subscriber: clayborg.

Just a few string reflows and fix a typo

lldb/source/Commands/CommandObjectBreakpointCommand.cpp
593 ↗(On Diff #218084)

replace multiple spaces with just one

lldb/source/Commands/CommandObjectTarget.cpp
2615–2617 ↗(On Diff #218084)

Fix string

lldb/source/Commands/CommandObjectWatchpointCommand.cpp
552–554 ↗(On Diff #218084)

reflow the string

lldb/source/Interpreter/CommandObject.cpp
929 ↗(On Diff #218084)

s/AnySe/AnySet/

This revision now requires changes to proceed.Aug 30 2019, 8:51 AM
teemperor updated this revision to Diff 218117.Aug 30 2019, 9:55 AM
teemperor marked 3 inline comments as done.
  • Fix typo in code and comment.
  • Reflow some changed strings.
teemperor marked an inline comment as done.Aug 30 2019, 9:55 AM
clayborg accepted this revision.Aug 30 2019, 1:21 PM
This revision is now accepted and ready to land.Aug 30 2019, 1:21 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2019, 2:40 AM