This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Commands] Add ability to run shell command on the host.
ClosedPublic

Authored by mib on May 8 2020, 6:41 PM.

Details

Summary

This patch introduces the (-h|--host) option to the platform shell
command. It allows the user to run shell commands from the host platform
(always available) without putting lldb in the background.

Since the default behaviour of platform shell is to run the command of
the selected platform, having such a choice can be quite handy when
debugging remote targets, for instances.

This patch also introduces a shell alias, to improve the command
discoverability and make it more convenient to use for the user.

rdar://62856024

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.May 8 2020, 6:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2020, 6:41 PM
mib edited the summary of this revision. (Show Details)May 8 2020, 6:55 PM
jingham added a subscriber: jingham.May 8 2020, 7:15 PM

We certainly shouldn't duplicate the code for CommandObjectPlatformShell.

Why don't you just add a "m_use_host_platform" ivar to CommandObjectPlatformShell, and make two instances of CommandObjectPlatformShell, one with m_use_host_platform set to false and added as a sub-command of platform, and another instance with m_use_host_platform to true, and add that as a base command "shell"? Then you'd just have to make CommandObjectPlatformShell switch on m_use_host_platform and either get the selected platform or the host platform.

You'd also have to adjust the help string for one or the other of the commands, but CommandObject::SetHelp and SetHelpLong will allow you to do that.

jingham requested changes to this revision.May 8 2020, 7:16 PM
This revision now requires changes to proceed.May 8 2020, 7:16 PM

I do something similar with CommandObjectThreadStepWithTypeAndScope so that I can share most of that command code, if you want to see an example of doing that.

mib updated this revision to Diff 262999.EditedMay 9 2020, 1:44 AM

With the first implementation, I thought it'd be a good idea to have separate Command Objects for platform shell and shell because I was imagining the latter could be more "powerful": shell could have an interactive mode like the script command, which might not be as relevant for platform shell when debugging a remote target.

That was my original idea, but I agree we should avoid code duplication, so here is a different implementation following @jingham's suggestions. shell is an alias to platform shell -h -- in this case.

mib edited the summary of this revision. (Show Details)May 9 2020, 1:44 AM
mib updated this revision to Diff 263005.May 9 2020, 2:33 AM
mib edited the summary of this revision. (Show Details)

It looks like command options are not supported on aliases that already have options:

(lldb) shell -t 1 -- sleep 5
zsh:1: command not found: -t
error: command returned with status 127

In this case, shell -t 1 -- sleep 5 expands to platform shell --host -- -t 1 -- sleep 5, and the option argument is passed as shell command instead of being parsed by lldb.

I might have done something wrong when creating the alias, so I'll skip the test_shell_timeout test until I come up with with a solution.

JDevlieghere added inline comments.May 9 2020, 11:02 AM
lldb/source/Commands/CommandObjectPlatform.cpp
1627

Is this only true for the alias?

lldb/test/API/commands/shell/TestShellCommand.py
27 ↗(On Diff #263005)

I assume you copied this from TestPlatformCommand.py but if the test is skipped on Windows we can probably remove this line here.

On that note, I think we could avoid some code duplication by merging these tests and have the same tests, once called with shell and once with platform shell.

mib marked an inline comment as done.May 11 2020, 8:58 AM
mib added inline comments.
lldb/source/Commands/CommandObjectPlatform.cpp
1627

I added this check because when using shell with no command, instead of showing the command usage, lldb tries running an empty command (if that makes sense) and showed the following error message:

(lldb) shell
error: executable doesn't exist: '(empty)'
mib updated this revision to Diff 264270.May 15 2020, 10:12 AM
mib marked 2 inline comments as done.

Merged the shell and platform shell test as @JDevlieghere suggested it.

JDevlieghere accepted this revision.May 15 2020, 12:12 PM

LGTM

lldb/source/Commands/CommandObjectPlatform.cpp
1590

Should we reset the timeout too?

This revision was not accepted when it landed; it landed in state Needs Review.May 15 2020, 1:37 PM
This revision was automatically updated to reflect the committed changes.
mib marked an inline comment as done.