Page MenuHomePhabricator

[asan] Add a "dump_registers" flag to print out CPU registers after a SIGSEGV
ClosedPublic

Authored by kubamracek on Jul 20 2015, 12:34 PM.

Details

Summary

This patch adds a new ASan flag, dump_registers (off by default), which after a SIGSEGV prints out all CPU registers. These are available in the signal handler context. The use case is when you're not running under the debugger, and you end up with a SIGSEGV ASan report, which doesn't contain much information – knowing the register values can be helpful here. Another interesting case is on x86_64, where if you fault on a non-canonical address (e.g. 0xdeaddeaddeaddead), this address is not propagated into siginfo->si_addr and is only located in one of the registers.

I only implemented the Darwin part and left the other functions (Linux, Windows) empty in hopes that some good soul will fill these in :)

Diff Detail

Repository
rL LLVM

Event Timeline

kubamracek retitled this revision from to [asan] Add a "dump_registers" flag to print out CPU registers after a SIGSEGV.
kubamracek updated this object.
kubamracek added reviewers: glider, samsonov, kcc.
kubamracek added subscribers: llvm-commits, kcc, samsonov and 2 others.
kcc edited edge metadata.Jul 20 2015, 1:16 PM

Looks good in general, still a few comments

lib/asan/asan_flags.inc
146 ↗(On Diff #30179)

Add something like "OSX-only for now"

lib/sanitizer_common/sanitizer_linux.cc
1102 ↗(On Diff #30179)

Add something like "Printf("DumpAllRegisters is not implemented on this platform");

same below.

lib/sanitizer_common/sanitizer_mac.cc
438 ↗(On Diff #30179)

#undef the macros at the end of the function

test/asan/TestCases/Darwin/dump_registers.cc
1 ↗(On Diff #30179)

I wonder if you can move this test one level up (not Darwin-specific) and restrict it for Darwin.

glider accepted this revision.Jul 21 2015, 2:17 AM
glider edited edge metadata.

LGTM once comments from Kostya are fixed.

This revision is now accepted and ready to land.Jul 21 2015, 2:18 AM
kubamracek updated this revision to Diff 30237.Jul 21 2015, 2:36 AM
kubamracek edited edge metadata.

Addressing review comments.

I wonder if you can move this test one level up (not Darwin-specific) and restrict it for Darwin.

Hm, it doesn't seem so. REQUIRES: Darwin doesn't work. I could add XFAIL: Linux and XFAIL: FreeBSD...

Also, what do you think about having this on by default?

kcc added a comment.Jul 21 2015, 6:23 PM
In D11365#208870, @kubabrecka wrote:

Addressing review comments.

I wonder if you can move this test one level up (not Darwin-specific) and restrict it for Darwin.

Hm, it doesn't seem so. REQUIRES: Darwin doesn't work. I could add XFAIL: Linux and XFAIL: FreeBSD...

Unless Alexey can suggest how to require Darwin, add a bunch of XFAILs.

Also, what do you think about having this on by default?

I don't mind at all, good idea.

kubamracek updated this revision to Diff 30489.Jul 23 2015, 8:24 AM

Added the XFAILs. Turning the flag on by default.

samsonov added inline comments.Jul 23 2015, 3:32 PM
lib/sanitizer_common/sanitizer_linux.cc
1102 ↗(On Diff #30489)

IMO you should either remove this line to not litter the output, or avoid enabling dump_registers by default...

lib/sanitizer_common/sanitizer_win.cc
663 ↗(On Diff #30489)

ditto

kubamracek updated this revision to Diff 30570.Jul 24 2015, 6:13 AM

IMO you should either remove this line to not litter the output, or avoid enabling dump_registers by default...

Makes sense. In that case it seems to me that it's better to just drop the flag altogether, and simply print this when it's implemented (OS X only for now).

kcc added a comment.Jul 27 2015, 11:38 AM

I'd keep the flag, just make it on by default.

kubamracek updated this revision to Diff 30819.Jul 28 2015, 8:02 AM

Addressing review comments (keeping the flag, on by default, removing output when not implemented).

This revision was automatically updated to reflect the committed changes.