labath added inline comments.
================ Comment at: lldb/lit/Host/TestCustomShell.test:5 +# RUN: SHELL=bogus %lldb %t.out -b -o 'run' 2>&1 | FileCheck %s +# CHECK: error: shell expansion failed ---------------- JDevlieghere wrote: > JDevlieghere wrote: > > friss wrote: > > > 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? > > 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? > Assuming we can find something that expands differently without throwing an > error in bash and zsh. 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). ================ Comment at: lldb/source/Host/posix/HostInfoPosix.cpp:119 + return FileSpec(v); + if (const char *v = ::getpwuid(::geteuid())->pw_shell) + return FileSpec(v); ---------------- friss wrote: > the dereference seems a little careless. getpwuid can fail and return NULL. 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68316/new/ https://reviews.llvm.org/D68316 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
