Escape command line arguments in backtraces

[ldrumm]

Summary

A common routine is to have the compiler crash, and attempt to rerun the
cc1 command-line by copying and pasting the arguments printed by
llvm::Support::PrettyStackProgram::print. However, these arguments are
not quoted or escaped which means they must be manually edited before
working correctly. This patch ensures that shell-unfriendly characters
are C-escaped, and arguments with spaces are double-quoted reducing the
frustration of running cc1 inside a debugger.

As the quoting is C, this is "best effort for most shells", but should
be fine for at least bash, zsh, csh, and cmd.exe.

[ldrumm]

An example of the output:

shell-session
 ./build-no-lto/bin/llc 'my file with spaces'          
LLVM ERROR: gimme a backtrace
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.      Program arguments: ./build-no-lto/bin/llc "my file with spaces" 
 #0 0x0000555b578b4f8d llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (./build-no-lto/bin/llc+0x1584f8d)
 #1 0x0000555b578b2d84 llvm::sys::RunSignalHandlers() (./build-no-lto/bin/llc+0x1582d84)
 #2 0x0000555b578b2ee0 SignalHandler(int) (./build-no-lto/bin/llc+0x1582ee0)
 #3 0x00007f6c739b1140 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14140)
 #4 0x00007f6c73494c41 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:51:1
 #5 0x00007f6c7347e537 abort ./stdlib/abort.c:81:7
 #6 0x0000555b5782fc2c llvm::report_fatal_error(llvm::Twine const&, bool) (./build-no-lto/bin/llc+0x14ffc2c)
 #7 0x0000555b5782fd38 (./build-no-lto/bin/llc+0x14ffd38)
 #8 0x0000555b5667c18c main (./build-no-lto/bin/llc+0x34c18c)
 #9 0x00007f6c7347fcca __libc_start_main ./csu/../csu/libc-start.c:308:16
#10 0x0000555b566cf84a _start (./build-no-lto/bin/llc+0x39f84a)
zsh: abort      ./build-no-lto/bin/llc 'my file with spaces'

[jhenderson]

Hi @ldrumm, could you reupload your patch with full context, as per the LLVM docs, please?

[ldrumm]

included full diff context

[jhenderson]

Seems reasonable overall. Just a coupel of nits.

llvm/lib/Support/PrettyStackTrace.cpp

257

What's the reason for changing the type of I? It's being used as an incrementing array index, so size_t seems like the most appropriate thing, if you're going to change it at all.

264

It would be nice to not have this trailing space after the last argument.

[ldrumm]

I absent mindedly responded to this review via email, without actually checking whether Phabricator supports this. Apologies for the delay

llvm/lib/Support/PrettyStackTrace.cpp

257

I changed it mostly because ArgC is signed, and signed / unsigned
comparisons make some people (and compilers) unhappy. The bounds of the
number of programming arguments are strictly within the range of an int,
so I don't think size_t buys anything.

264

Agreed. I'll fix this up

[jhenderson]

llvm/lib/Support/PrettyStackTrace.cpp

257

int is fine, thanks. Didn't register that it was a strict tracking of the argc from main.

[ldrumm]

Patch updated to not insert trailing blank

[jhenderson]

LGTM!

llvm/lib/Support/PrettyStackTrace.cpp

 #include <cassert>
 #include <cstdarg>
 #include <cstdio>
+#include <cstring>
 #include <tuple>
 
 #ifdef HAVE_CRASHREPORTERCLIENT_H

 void PrettyStackTraceProgram::print(raw_ostream &OS) const {
   OS << "Program arguments: ";
   // Print the argument list.
-  for (unsigned i = 0, e = ArgC; i != e; ++i)
-    OS << ArgV[i] << ' ';
+  for (int I = 0; I < ArgC; ++I) {
+    const bool HaveSpace = ::strchr(ArgV[I], ' ');
+    if (HaveSpace)
+      OS << '"';
+    OS.write_escaped(ArgV[I]);
+    if (HaveSpace)
+      OS << '"';
+    OS << ' ';
+  }
   OS << '\n';
 }