This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Target] Add custom interpreter option to `platform shell`
ClosedPublic

Authored by mib on Aug 26 2020, 6:13 PM.

Details

Summary

This patch adds the ability to use a custom interpreter with the
platform shell command. If the user set the -s|--shell option
with the path to a binary, lldb passes it down to the platform's
RunShellProcess method and set it as the shell to use in
`ProcessLaunchInfo to run commands.

Note that not all the Platforms support running shell commands with
custom interpreters (i.e. RemoteGDBServer is only expected to use the
default shell).

This patch also makes some refactoring and cleanups, like swapping
CString for StringRef when possible and updating SBPlatformShellCommand
with new methods and a new constructor.

rdar://67759256

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

Diff Detail

Event Timeline

mib created this revision.Aug 26 2020, 6:13 PM
mib requested review of this revision.Aug 26 2020, 6:13 PM
mib updated this revision to Diff 288147.Aug 26 2020, 6:15 PM
JDevlieghere added inline comments.Aug 26 2020, 7:44 PM
lldb/source/API/SBPlatform.cpp
59

Given that this pattern repeats a few times in this struct, maybe a small static helper function would be nice:

static bool is_not_empty(cont char* c) { return c && c[0]; }
64

Shouldn't we just bail out if the command is null/empty? Now it will run shell -c, right?

71

m_command is a std::string, why call c_str()?

kastiglione added inline comments.
lldb/source/API/SBPlatform.cpp
59

Can these be StringRef, and then check with empty()?

lldb/source/Commands/CommandObjectPlatform.cpp
1627

Is it necessary to check readability? If not done here, at execution time would a non-readable interpreter lead to a useful error message?

If checking for readability is desirable here, is checking that it's executable also desirable?

lldb/test/API/commands/platform/basic/TestPlatformCommand.py
110

I am wondering what should the platform classes which do not implement this feature do (although most of them could implemented I don't think you need to implement all of them -- PlatformGdbRemote in particular can be a bit tricky). It seems to me like it would be better for them to bail out if a non-standard shell is requested instead of silently executing a command the user did not intend.

lldb/include/lldb/Target/Platform.h
644

const on a value parameter is useless. Also, default arguments on virtual methods are somewhat shoot-footy: http://www.cplusplus.com/forum/general/69970/. The Non-virtual interface pattern would solve that, and maybe have some other benefits.

lldb/source/API/SBPlatform.cpp
59–68

Rather than duplicating this logic here (and getting it wrong for windows, for nested quotes, etc.) it would be better to just pass the custom shell to the RunShellCommand method. Then it use the existing quoting logic, and all that needs to change is that it now picks up the shell from the argument (if passed) instead of the baked-in default.

lldb/source/Commands/CommandObjectPlatform.cpp
1621–1633

This won't work for remote platforms. The checks should be at least done in the Platform class, but ideally I would really just leave it to the operating system to figure out if it can or cannot run a given command.

lldb/test/API/commands/platform/basic/TestPlatformCommand.py
97

You could build a small executable to serve as the "shell". Then, this test would work everywhere.

Do people really call command-line shells interpreters? I would have thought --shell would be a better name. lldb already has its own command interpreter which is orthogonal to the shells, so it seems confusing to use the same term for both.

mib marked 9 inline comments as done.Aug 27 2020, 9:51 PM

Do people really call command-line shells interpreters? I would have thought --shell would be a better name. lldb already has its own command interpreter which is orthogonal to the shells, so it seems confusing to use the same term for both.

I think platform shell --shell sounds/looks repetitive so I opted for -i|--interpreter instead, as in Command-line Interpreter.

lldb/source/API/SBPlatform.cpp
59

I think since this method will eventually interact with the Python API, they need to be C strings.

lldb/test/API/commands/platform/basic/TestPlatformCommand.py
110

Since it's a command output, it has a newline character at the end of it.

mib updated this revision to Diff 288531.Aug 27 2020, 10:40 PM
mib marked an inline comment as done.
mib edited the summary of this revision. (Show Details)

Address most comments:

  • Passed down the shell interpreter path down to Host::RunShellCommand to be "assembled" with the rest of the command.
  • Wrote a custom test program to simplify testing on multiple platforms.
  • Updated code by using llvm::StringRef instead of CString when possible.
In D86667#2243781, @mib wrote:

Do people really call command-line shells interpreters? I would have thought --shell would be a better name. lldb already has its own command interpreter which is orthogonal to the shells, so it seems confusing to use the same term for both.

I think platform shell --shell sounds/looks repetitive so I opted for -i|--interpreter instead, as in Command-line Interpreter.

Yes, it is repetitive. OTOH:

  • the unix commands that take a shell as an argument (e.g., su, sudo), do so via a --shell/-s argument
  • we have a memory load --load command

Overall, I can understand where you're coming from, but I think I'd go with --shell nonetheless...

lldb/include/lldb/Target/Platform.h
654

The run_in_shell argument makes the inferface weird (why do I need to specify a shell, if I don't want to use it). And still suffers from the virtual default argument problem described above. Let's not propagate this Host weirdness into the platform class.

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
2815

These Shouldn't be NULL are out of place now, and probably not needed anymore.

2826

command.data() will do just fine

lldb/test/API/commands/platform/basic/TestPlatformPython.py
96

We normally set these things in the makefile. The environment tricks are reserved for cases where you need to do something funny (e.g. build multiple things from a single makefile, or similar...).

In D86667#2243781, @mib wrote:

Do people really call command-line shells interpreters? I would have thought --shell would be a better name. lldb already has its own command interpreter which is orthogonal to the shells, so it seems confusing to use the same term for both.

I think platform shell --shell sounds/looks repetitive so I opted for -i|--interpreter instead, as in Command-line Interpreter.

Yes, it is repetitive. OTOH:

  • the unix commands that take a shell as an argument (e.g., su, sudo), do so via a --shell/-s argument
  • we have a memory load --load command

Overall, I can understand where you're coming from, but I think I'd go with --shell nonetheless...

I'd agree, process launch already takes a -c|--shell argument so it would be consistent to do that. (I assume -c was used because it's kind of like sh(1)'s -c option but not really. maybe more likely that -s was already used ;)

mib marked 3 inline comments as done.Aug 28 2020, 6:38 AM
mib added inline comments.
lldb/include/lldb/Target/Platform.h
654

I started by removing this argument, then I noticed that some code calls Host::RunShellCommand with the run_in_shell argument set to false (i.e. PlatformDarwin::GetXcodeSelectPath). This means that Host::RunShellCommand will run the command without prepending the "sh -c" part.

That's why I kept it, and renamed it to run_in_shell instead of run_in_default_shell.

mib marked 2 inline comments as done.Aug 28 2020, 6:39 AM
mib updated this revision to Diff 288593.Aug 28 2020, 6:42 AM
mib edited the summary of this revision. (Show Details)
  • Changed CommandObject option from -i|--interpreter to -s|--shell.
  • Updated test program build settings.
labath added inline comments.Aug 28 2020, 6:46 AM
lldb/include/lldb/Target/Platform.h
654

Right, but that code only calls the Host version. I'm only saying we should delete it (or, not add it, to be precise) to the Platform version. (I'd like to remove it from the Host version too, but that's a different story.)

mib updated this revision to Diff 288597.Aug 28 2020, 6:57 AM
mib marked an inline comment as done.
mib edited the summary of this revision. (Show Details)

Removed bool run_in_shell argument from Platform classes.

mib updated this revision to Diff 288636.Aug 28 2020, 9:29 AM

Replaced all the interpreter occurrences with shell in the code.

labath accepted this revision.Sep 2 2020, 7:31 AM

Thanks, this looks good to me now.

lldb/test/API/commands/platform/basic/TestPlatformPython.py
16

If you add this, then there's no need for @no_debug_info_test on individual test methods.

This revision is now accepted and ready to land.Sep 2 2020, 7:31 AM
This revision was landed with ongoing or failed builds.Sep 2 2020, 7:37 AM
This revision was automatically updated to reflect the committed changes.

Hi @mib, Windows buildbot is broken after this commit. Please fix or revert.

Failed Tests (2):

lldb-api :: commands/platform/basic/TestPlatformCommand.py
lldb-api :: commands/platform/basic/TestPlatformPython.py

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/18671/steps/test/logs/stdio

http://lab.llvm.org:8011/buildslaves/win-py3-buildbot

mib added a comment.Sep 2 2020, 11:12 AM

@max-kudr Looking, thanks!

mib added inline comments.Sep 2 2020, 12:26 PM
lldb/test/API/commands/platform/basic/myshell.c
12

@max-kudr It seems I'm missing the leading _ (correct me if I'm wrong but IIUC the macro should be _WIN32), explaining the failure, I'll send a fix now.

max-kudr added inline comments.Sep 2 2020, 12:37 PM
lldb/test/API/commands/platform/basic/myshell.c
12

Yes, at least _WIN32 is used in clang. Thank you!

mib added a comment.Sep 2 2020, 12:43 PM

Should be fixed with 0e86f390457a2b4dd1f2d1770db912963a36f240. @max-kudr I'll keep an eye on the Windows bot to make sure it worked :)