This is an archive of the discontinued LLVM Phabricator instance.

[TargetList] Simplify dummy target creation
ClosedPublic

Authored by vsk on Nov 5 2020, 11:54 AM.

Details

Summary

Factor out dummy target creation from CreateTargetInternal.

This makes it impossible for dummy target creation to accidentally fail
due to too-strict checking in one of the CreateTargetInternal overloads.

Testing: check-lldb

rdar://70630655

Diff Detail

Event Timeline

vsk created this revision.Nov 5 2020, 11:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 5 2020, 11:54 AM
vsk requested review of this revision.Nov 5 2020, 11:54 AM
vsk updated this revision to Diff 303223.Nov 5 2020, 12:02 PM

Delete some dead code.

JDevlieghere added inline comments.Nov 5 2020, 1:08 PM
lldb/source/Target/TargetList.cpp
274–275

I wonder if we gain anything by making this lazy. Maybe it's time to address that FIXME and always have a single DummyTarget held onto by the Debugger?

vsk marked an inline comment as done.Nov 5 2020, 1:40 PM
vsk added inline comments.
lldb/source/Target/TargetList.cpp
274–275

You're right, that's even better.

vsk updated this revision to Diff 303252.Nov 5 2020, 1:40 PM
vsk marked an inline comment as done.

Make the dummy target per-debugger.

jingham added inline comments.Nov 5 2020, 1:40 PM
lldb/source/Target/TargetList.cpp
274–275

I see no reason not to just make a dummy target when you make a debugger. That should be a very cheap operation, and the object itself should not be large.

I think for regularities sake it's better to have the TargetList have all the targets, including the Dummy target, rather than have the DummyTarget held specially by the debugger. But it would be easy for the Debugger to make a DummyTarget and inject it into the TargetList as part of its startup. We could even make the straight constructor of TargetList do this job. We copy the debugger TargetList to iterate over it and for similar purposes in a bunch of places, but that goes through the copy constructor so it wouldn't interfere with this.

vsk added inline comments.Nov 5 2020, 1:43 PM
lldb/source/Target/TargetList.cpp
274–275

So far, the policy has been to not include the dummy target in the list of targets (i.e, you can't find it via TargetList::GetTargetAtIndex or TargetList::GetIndexOfTarget). If, down the line, we need to support that we can add the functionality though.

Oh, my bad, apparently we were leaving the dummy target out of the TargetList. That's a little odd, but I probably did it so that's not so unusual... Ignore my comments...

teemperor accepted this revision.Nov 5 2020, 2:18 PM

LGTM.

lldb/source/Core/Debugger.cpp
685–694

Maybe have a small comment here that summarizes the block: // Create dummy target?

This revision is now accepted and ready to land.Nov 5 2020, 2:18 PM
JDevlieghere added inline comments.Nov 5 2020, 2:33 PM
lldb/source/Core/Debugger.cpp
692

Should we move all this into Target and have a private static TargetSP GetDummyTarget()?

vsk added inline comments.Nov 5 2020, 2:51 PM
lldb/source/Core/Debugger.cpp
685–694

I'll either fix this in a more substantial patch revision or before committing.

692

I personally find that having the target creation inlined in the Debugger constructor is nice: it precludes some other code from setting up a separate dummy target.

JDevlieghere accepted this revision.Nov 5 2020, 3:02 PM
This revision was landed with ongoing or failed builds.Nov 5 2020, 4:04 PM
This revision was automatically updated to reflect the committed changes.