Page MenuHomePhabricator

UBSAN: emit distinctive traps in trapping mode
ClosedPublic

Authored by t.p.northover on Oct 22 2020, 6:44 AM.

Details

Reviewers
vsk
morehouse
Summary

When a trapping sanitizer is deployed in release configurations, you might have limited ability to get information back from crashes. This patch makes Clang emit a different kind of trap, firstly for UBSAN compared to any other reason we might trap, but also for its different failure modes.

A crash-dumping program can then inspect the opcode and give you, if not a truly specific diagnosis, at least a function and what kind of thing to look for which is often enough to diagnose the problem.

Because of the extra traps there is a small code-size penalty, but it's pretty small compared to what we accept just for the sanitization so probably not a big concern (57.0% overhead as opposed to 55.7% currently on SPEC, for AArch64),

For now I've implemented the new kind of trap for AArch64 and X86, with fallback to a default trap elsewhere.

Diff Detail

Event Timeline

t.p.northover created this revision.Oct 22 2020, 6:44 AM
t.p.northover requested review of this revision.Oct 22 2020, 6:44 AM

Missing langref changes for new intrinsic.

vsk added a subscriber: vsk.Oct 22 2020, 2:08 PM

Oh yes, added now.

Friendly ping

vsk added inline comments.Oct 29 2020, 9:30 AM
clang/lib/CodeGen/CGExpr.cpp
3451–3455

'per check, per function'?

3462

This seems to apply the current DebugLoc from Builder to the shared trap call when optimizing. That's potentially misleading (say you have two trapping additions -- if the second one traps, the crashlog will make it look like the first one trapped).

I think the fix is just: if (optimizing) TrapCall->dropLocation();. This can be fixed before/after/in this patch, whatever you prefer.

llvm/lib/Target/AArch64/AArch64InstrInfo.td
6706

assert(N->getZExtValue() < 256)? Or alternatively, maybe it'd be simpler to define @llvm.ubsantrap(i8 immarg)?

vsk added inline comments.Oct 29 2020, 9:42 AM
clang/lib/CodeGen/CGExpr.cpp
3462

Or better: instead of dropping the debug loc of the trap call immediately, apply a merged debug loc before emitting the condbr (see Instruction::applyMergedLocation). That way, if there aren't multiple trapping operations, the original debug loc is preserved.

t.p.northover marked 3 inline comments as done.

Thanks. This switches to i8 argument so there's no possibility of ignoring bits and merges the debug info as suggested.

tschuett added inline comments.
llvm/docs/LangRef.rst
19494

This is still i32.

vsk added a comment.Nov 2 2020, 8:09 AM

I might've missed it, but the debug location merging behavior could use a test. Here's one way to write it: https://godbolt.org/z/Yb9PY9.

In @_Z13keep_locationPi, the !dbg attachment on the trap should match !DILocation(line: 1. In @_Z15merge_locationsPi, the attachment should match !DILocation(line: 0 (it currently incorrectly points to line 4).

Added test for debug-loc merging (ubsan-trap-debugloc.c).

vsk accepted this revision.Nov 3 2020, 9:11 AM
vsk added subscribers: kcc, eugenis.

Thanks, lgtm with two comments --

Because of the extra traps there is a small code-size penalty, but it's pretty small compared to what we accept just for the sanitization so probably not a big concern (57.0% overhead as opposed to 55.7% currently on SPEC, for AArch64),

Was this measured with all of -fsanitize=undefined enabled? If so, the actual size overhead is likely lower, as only a subset of these checks get enabled in production settings.

llvm/docs/LangRef.rst
19494

Yes, please update the langref.

This revision is now accepted and ready to land.Nov 3 2020, 9:11 AM

did you consider approaches where the emitted code doesn't change, but the binary contains a debug-like metadata that corresponds to the trap instructions?
Matt (CC-ed) has a patch if this kind (for a different purpose) in the works .

With current CodeGen by the time you reach the trap you have no idea what came before so I think you'd still need a separate trap instruction per failure kind. So the Clang and generic LLVM side would be unaffected.

I suppose on X86 it could save a few bytes in .text and so maybe icache (though again we're talking about roughly 50% bloat for enabling UBSAN at all so this is kind of lost in the noise). On the whole I'm not a fan of inventing a new entirely separate scheme for that.

t.p.northover marked an inline comment as done.Nov 5 2020, 1:52 AM

Was this measured with all of -fsanitize=undefined enabled? If so, the actual size overhead is likely lower, as only a subset of these checks get enabled in production settings.

Yes, I used -fsanitize=undefined -fsanitize-trap=all. Fixed the LangRef but I won't upload another diff unless something more substantial changes.

jdoerfert added inline comments.Nov 5 2020, 1:58 PM
llvm/include/llvm/IR/Intrinsics.td
1242

should this be readonly and inaccesiblememonly?

t.p.northover added inline comments.Nov 9 2020, 7:10 AM
llvm/include/llvm/IR/Intrinsics.td
1242

For readonly I'd say no (LangRef says "if a readonly function [...] has other side-effects, the behavior is undefined"), inaccessiblememonly maybe, but I doubt the definitions were written with something like a trap in mind so the whole topic seems pretty fuzzy to me.

t.p.northover closed this revision.Dec 8 2020, 2:29 AM

Committed:

To github.com:llvm/llvm-project.git

c54d827fdb12..c5978f42ec8e  main -> main