Page MenuHomePhabricator

Escape command line arguments in backtraces
ClosedPublic

Authored by ldrumm on Nov 4 2020, 4:51 AM.

Details

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.

Diff Detail

Event Timeline

ldrumm created this revision.Nov 4 2020, 4:51 AM
ldrumm requested review of this revision.Nov 4 2020, 4:51 AM

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'

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

ldrumm updated this revision to Diff 302824.Nov 4 2020, 5:45 AM

included full diff context

Seems reasonable overall. Just a coupel of nits.

llvm/lib/Support/PrettyStackTrace.cpp
257–266

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.

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–266

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 added inline comments.Nov 16 2020, 12:45 AM
llvm/lib/Support/PrettyStackTrace.cpp
257–266

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

ldrumm updated this revision to Diff 305556.Nov 16 2020, 10:30 AM
ldrumm marked 2 inline comments as not done.

Patch updated to not insert trailing blank

ldrumm marked 2 inline comments as done.Nov 16 2020, 10:31 AM
This revision is now accepted and ready to land.Nov 17 2020, 12:24 AM
This revision was automatically updated to reflect the committed changes.