Page MenuHomePhabricator

[lldb] Fix invalid error message in TargetList::CreateTargetInternal
ClosedPublic

Authored by JDevlieghere on Jul 28 2020, 4:13 PM.

Details

Summary

Currently, target create has no --platform option. However, TargetList::CreateTargetInternal which is called under the hood will return an error when either no platform or more than one is found that matches the given binary.

This patch adds the platform option, but that doesn't solve either of these errors. If more than one platform matches, specifying the platform isn't going to fix that. The current code will look at the architecture instead. So I've updated the error message to ask the user to specify an architecture. If no architecture is found, specifying a new one via platform isn't going to change that either because we already try to find one that matches the given architecture.

While I was there I also did some general cleanup/refactoring and improve the test coverage.

Diff Detail

Event Timeline

JDevlieghere created this revision.Jul 28 2020, 4:13 PM
JDevlieghere requested review of this revision.Jul 28 2020, 4:13 PM
JDevlieghere edited the summary of this revision. (Show Details)Jul 28 2020, 4:41 PM
JDevlieghere edited reviewers, added: labath, clayborg, friss; removed: jdoerfert.
JDevlieghere retitled this revision from [lldb] Various related target create improvements to [lldb] Fix invalid error message in TargetList::CreateTargetInternal.Jul 28 2020, 4:42 PM

This LGTM with a couple of quibbles but I haven't been working on the platform support recently so we should wait till people who have done so weight in.

lldb/source/Target/TargetList.cpp
175–176

This comment was confusing before, but it's more confusing now because platform_sp might be the user specified platform or it might be the one passed in with --platform.

245

I'm not sure why you removed mention of --platform from this error because with this patch it should now be possible to disambiguate either by specifying --platform or --arch, right?

lldb/test/API/commands/target/basic/bogus.yaml
946

You don't use the debug info, might as well leave it out.

jingham added inline comments.Jul 28 2020, 5:02 PM
lldb/source/Target/TargetList.cpp
245

Ah, okay I didn't see that the platform doesn't get used everywhere in this patch.

lldb/test/API/commands/target/basic/bogus.yaml
946

Done...

JDevlieghere added inline comments.Jul 28 2020, 5:52 PM
lldb/source/Target/TargetList.cpp
245

Another reason is that this is generic code, it shouldn't mention command interpreter options. You could've called in here trough the SB API as well.

The changes don't seem contentious to me, though I am also not very familiar with this code.

What surprised me was the OptionGroupPlatform argument to TargetList::CreateTargetInternal. It seems like a slighly unusual thing to have, as that means the equivalent SB call would have to construct an OptionGroup object even though it has nothing to do with CLI. But that is not a problem caused by this patch...

This revision was not accepted when it landed; it landed in state Needs Review.Jul 29 2020, 10:30 AM
This revision was automatically updated to reflect the committed changes.
JDevlieghere marked 3 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2020, 10:30 AM