[lldb] Escape ? for zsh

[keith]

Previously on macOS with lldb-argdumper if you ran:

lldb -o r -- /tmp/foo "some arg?"

It would fail with this error:

error: shell expansion failed (reason: lldb-argdumper exited with error 1). consider launching with 'process launch'.

stderr is silenced here but the underlying error if you print it for
debugging is:

zsh: no matches found: ?

This change escapes the ? so this argument works as expected.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Keith Smiley (keith)

Changes

Previously on macOS with lldb-argdumper if you ran:

lldb -o r -- /tmp/foo "some arg?"

It would fail with this error:

error: shell expansion failed (reason: lldb-argdumper exited with error 1). consider launching with 'process launch'.

stderr is silenced here but the underlying error if you print it for
debugging is:

zsh: no matches found: ?

This change escapes the ? so this argument works as expected.

---

Full diff: https://github.com/llvm/llvm-project/pull/112107.diff

2 Files Affected:

• (modified) lldb/source/Utility/Args.cpp (+1-1)
• (modified) lldb/unittests/Utility/ArgsTest.cpp (+2-2)

diff --git a/lldb/source/Utility/Args.cpp b/lldb/source/Utility/Args.cpp
index 8ba40bae4d67e5..92a7ef03273fbc 100644
--- a/lldb/source/Utility/Args.cpp
+++ b/lldb/source/Utility/Args.cpp
@@ -401,7 +401,7 @@ std::string Args::GetShellSafeArgument(const FileSpec &shell,
   static ShellDescriptor g_Shells[] = {{"bash", " '\"<>()&;"},
                                        {"fish", " '\"<>()&\\|;"},
                                        {"tcsh", " '\"<>()&;"},
-                                       {"zsh", " '\"<>()&;\\|"},
+                                       {"zsh", " '\"<>()&;\\|?"},
                                        {"sh", " '\"<>()&;"}};
 
   // safe minimal set
diff --git a/lldb/unittests/Utility/ArgsTest.cpp b/lldb/unittests/Utility/ArgsTest.cpp
index 8d2b625f524d67..34d6b4dd7c95a0 100644
--- a/lldb/unittests/Utility/ArgsTest.cpp
+++ b/lldb/unittests/Utility/ArgsTest.cpp
@@ -292,8 +292,8 @@ TEST(ArgsTest, GetShellSafeArgument) {
   EXPECT_EQ(Args::GetShellSafeArgument(bash, "a\"b"), "a\\\"b");
 
   FileSpec zsh("/bin/zsh", FileSpec::Style::posix);
-  EXPECT_EQ(Args::GetShellSafeArgument(zsh, R"('";()<>&|\)"),
-            R"(\'\"\;\(\)\<\>\&\|\\)");
+  EXPECT_EQ(Args::GetShellSafeArgument(zsh, R"('"?;()<>&|\)"),
+            R"(\'\"\?\;\(\)\<\>\&\|\\)");
   // Normal characters and expressions that shouldn't be escaped.
   EXPECT_EQ(Args::GetShellSafeArgument(zsh, "aA$1*"), "aA$1*");
 

[labath]

So... ? is a glob character (in just about any shell), just like *. And lldb-argdumper's only reason for existence is to expand globs, so escaping it sounds wrong. I think the main difference here is that zsh complains loudly about a failed glob expansion, whereas bash just leaves it alone (although this can be controlled with some settings). However, this behavior is not specific to ? -- you can get the same thing with * as well:

$ zsh -c 'echo bin/lld?'
bin/lldb
$ zsh -c 'echo bin/lld?'
bin/lldb
$ zsh -c 'echo bin/lld*'
bin/lld bin/lld-link bin/lldb bin/lldb-argdumper bin/lldb-dap bin/lldb-dotest bin/lldb-instr bin/lldb-python bin/lldb-repro bin/lldb-server bin/lldb-tblgen bin/lldb-test
$ zsh -c 'echo bin/not-lld?'
zsh:1: no matches found: bin/not-lld?
$ zsh -c 'echo bin/not-lld*'
zsh:1: no matches found: bin/not-lld*
$ bash -c 'echo bin/lld?'
bin/lldb
$ bash -c 'echo bin/lld*'
bin/lld bin/lld-link bin/lldb bin/lldb-argdumper bin/lldb-dap bin/lldb-dotest bin/lldb-instr bin/lldb-python bin/lldb-repro bin/lldb-server bin/lldb-tblgen bin/lldb-test
$ bash -c 'echo bin/not-lld*'
bin/not-lld*
$ bash -c 'echo bin/not-lld?'
bin/not-lld?

[keith]

hmm good point. I wonder if the fix here should be around surfacing the error to users then, so they know they have to escape it depending on their use case

[kastiglione]

so they know they have to escape it depending on their use case

in the example shown, "some arg?" is quoted, which already escapes the glob. Do we really need to force an additional level of escaping on users? Hopefully not but I can see that this is tricky.

[keith]

Oh yea great point. I guess another way to put that is if I copy the exact same invocation and run it directly it works, so that differing is unexpected.

[labath]

I agree (and I don't think this is going to be very contentious) that we should not expand args from the lldb command line (precisely for the reason you mention). The trickiness comes from how this expansion is performed. The command line args are first copied into the target.run-args setting, and only later expanded when you actually type "run". The easiest thing to do would be to say that we never expand arguments that come from the setting, but that means something like this wouldn't be expanded either:

(lldb) settings set target.run-args -- *.txt # I am setting this manually and want this to be expanded
(lldb) run # no expansion

I would (probably?) be fine with that interpretation, but I suspect others will have different opinions. If we want command line args to not get expanded, but keep expanding the example above, then we may need to come up with a different way to pass cmdline args.