This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Avoid using any shell when calling xcrun.
ClosedPublic

Authored by teemperor on Jun 21 2021, 8:54 AM.

Details

Summary

When we run xcrun we don't have any user input in our command so relying on the user's default
shell doesn't make a lot of sense. If the user has set the system shell to a something that isn't supported
yet (dash, ash) then we would run into the problem that we don't know how to escape our command string.

This patch just avoids using any shell at all as xcrun is always at the same path.

Diff Detail

Event Timeline

teemperor requested review of this revision.Jun 21 2021, 8:54 AM
teemperor created this revision.
teemperor retitled this revision from [lldb] Explicitly prefer a vanilla shell (zsh, bash, sh) over the user's default shell to [lldb] Explicitly prefer a vanilla shell (zsh, bash, sh) over the user's default shell when running `xcrun`.

FWIW, I'm open to preferring bash over zsh as the default shell. But as zsh is already the default on macOS we might as well start using it when possible.

Is there a utility for exec'ing a command without a shell? Seems this could also be solved by avoiding a shell altogether.

aprantl added inline comments.Jun 22 2021, 11:25 AM
lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
385–386

I think this should always be /usr/bin/xcrun anyway, and then we can as @kastiglione suggested avoid using a shell altogether, right?

teemperor retitled this revision from [lldb] Explicitly prefer a vanilla shell (zsh, bash, sh) over the user's default shell when running `xcrun` to [lldb] Avoid using any shell when calling xcrun..
teemperor edited the summary of this revision. (Show Details)
  • Addressing Adrian's comments (sorry for the delay)
teemperor added a comment.EditedJun 22 2021, 11:27 AM

@kastiglione Good point, I dropped using any shell for the command.

aprantl added inline comments.Jun 22 2021, 11:32 AM
lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
401

Can we increase this to something much larger? It's only supposed to guard against catastrophic failure and xcrun is known to take a surprising amount of time the first tie it's launched.

This revision is now accepted and ready to land.Jun 22 2021, 11:34 AM
aprantl accepted this revision.Jun 22 2021, 4:27 PM
This revision was landed with ongoing or failed builds.Jun 28 2021, 10:54 AM
This revision was automatically updated to reflect the committed changes.