[Host] Return the user's shell from GetDefaultShell

[JDevlieghere]

Summary

LLDB handles shell expansion by running lldb-argdumper under a shell. Currently, this is always /bin/sh on POSIX. This potentially leads to different behavior between lldb and the user's current shell. Here's an example of different expansions between shells:

$ /bin/bash -c 'echo -config={Options:[{key:foo_key,value:foo_value}]}'
-config={Options:[key:foo_key]} -config={Options:[value:foo_value]}
$ /bin/zsh -c 'echo -config={Options:[{key:foo_key,value:foo_value}]}'
zsh:1: no matches found: -config={Options:[key:foo_key]}
$ /bin/sh -c 'echo -config={Options:[{key:foo_key,value:foo_value}]}'
-config={Options:[key:foo_key]} -config={Options:[value:foo_value]}
$ /bin/fish -c 'echo -config={Options:[{key:foo_key,value:foo_value}]}'
-config=Options:[key:foo_key] -config=Options:[value:foo_value]

To reduce surprises, this patch returns the user's current shell. It first looks at the SHELL environment variable. If that isn't set, it'll ask for the user's default shell. Only if that fails, we'll fallback to /bin/sh, which should always be available.

[friss]

lldb/lit/Host/TestCustomShell.test

5

Is there a reliable way to check that the expansion we get in lldb matches the one in the shell? For example, could we have the program dump its arguments once without lldb and match them against the lldb output?

I guess the zsh example that errors out in your description makes this hard?

lldb/source/Host/posix/HostInfoPosix.cpp

119

the dereference seems a little careless. getpwuid can fail and return NULL.

[JDevlieghere]

Check pointer returned by getpwuid.

[JDevlieghere]

lldb/lit/Host/TestCustomShell.test

5

You could run lldb-argdumper under lldb and compare the output to running it under different shells. Do you think it's reasonable to assume that at least /bin/bash and /bin/zsh are available?

[JDevlieghere]

lldb/lit/Host/TestCustomShell.test

5

Assuming we can find something that expands differently without throwing an error in bash and zsh.

[labath]

lldb/lit/Host/TestCustomShell.test

5

I don't think you can assume zsh is universally available. If you wanted to do something like, the safest option would be to write a mock shell, which implements some dumb substitution (s/FOO/BAR/) and otherwise forwards to the real shell. But, I wouldn't say that is required, as that is not even testing what this patch is changing. A simpler but still interesting test might be to unset the SHELL variable to ensure that the getpwuid path is at least executed (as one can't really check it's result reasonably).

lldb/source/Host/posix/HostInfoPosix.cpp

119

Also, getpwuid is not thread safe. There's already code in this very file which chooses between getpwuid and getpwuid_r, but it is hidden inside PosixUserIDResolver::DoGetUserName. If you factor that out into a helper function, you'll be able to always use the best available API.

lldb/lit/Host/Inputs/simple.c

+int main(int argc, char const *argv[]) { return 0; }

lldb/lit/Host/TestCustomShell.test

+# UNSUPPORTED: system-windows
+
+# RUN: %clang %S/Inputs/simple.c -g -o %t.out
+# RUN: SHELL=bogus %lldb %t.out -b -o 'run' 2>&1 | FileCheck %s
+# CHECK: error: shell expansion failed

lldb/source/Host/posix/HostInfoPosix.cpp

 uint32_t HostInfoPosix::GetUserID() { return getuid(); }
 
 uint32_t HostInfoPosix::GetGroupID() { return getgid(); }
 
 uint32_t HostInfoPosix::GetEffectiveUserID() { return geteuid(); }
 
 uint32_t HostInfoPosix::GetEffectiveGroupID() { return getegid(); }
 
-FileSpec HostInfoPosix::GetDefaultShell() { return FileSpec("/bin/sh"); }
+FileSpec HostInfoPosix::GetDefaultShell() {
+  if (const char *v = ::getenv("SHELL"))
+    return FileSpec(v);
+  if (const char *v = ::getpwuid(::geteuid())->pw_shell)
+    return FileSpec(v);
+  return FileSpec("/bin/sh");
+}
 
 bool HostInfoPosix::ComputeSupportExeDirectory(FileSpec &file_spec) {
   return ComputePathRelativeToLibrary(file_spec, "/bin");
 }
 
 bool HostInfoPosix::ComputeHeaderDirectory(FileSpec &file_spec) {
   FileSpec temp_file("/opt/local/include/lldb");
   file_spec.GetDirectory().SetCString(temp_file.GetPath().c_str());