This is an archive of the discontinued LLVM Phabricator instance.

[ubsan] Reduce alignment checking of C++ object pointers
ClosedPublic

Authored by vsk on Feb 22 2017, 5:39 PM.

Details

Summary

This patch teaches ubsan to insert an alignment check for the 'this'
pointer at the start of each method/lambda. This allows clang to emit
significantly fewer alignment checks overall, because if 'this' is
aligned, so are its fields.

This is essentially the same thing r295515 does, but for the alignment
check instead of the null check. One difference is that we keep the
alignment checks on member expressions where the base is a DeclRefExpr.
There's an opportunity to diagnose unaligned accesses in this situation
(as pointed out by Eli, see PR32630).

Testing: check-clang, check-ubsan, and a stage2 ubsan build.

Along with the patch from D30285, this roughly halves the amount of
alignment checks we emit when compiling X86FastISel.cpp. Here are the
numbers from patched/unpatched clangs based on r298160.

Setup# of alignment checks
unpatched, -O024326
patched, -O012717(-47.7%)

There are a few possible follow-ups:

  • Don't add the per method/lambda check in delegating constructors.
  • Don't instrument accesses to fields with alignment = 1.

Diff Detail

Repository
rL LLVM

Event Timeline

vsk created this revision.Feb 22 2017, 5:39 PM
vsk added a reviewer: rsmith.Mar 10 2017, 11:51 AM

Ping. I appreciate that there are a lot of test changes to sift through here -- please let me know if I can make the patch easier to review in any way.

I'm not sure we actually want to skip these checks for DeclRefExps. I mean, you can rely on the backend to correctly align a local variable (assuming the stack is correctly aligned), but it's very easy to misalign a global.

vsk added a comment.Mar 21 2017, 3:19 PM

Hi Eli, thanks for your feedback :).

I'm not sure we actually want to skip these checks for DeclRefExps. I mean, you can rely on the backend to correctly align a local variable (assuming the stack is correctly aligned), but it's very easy to misalign a global.

I did some experimenting, and think that we can safely skip the alignment check for DeclRefExps. Here is what I found:

  1. It seems hard to declare a misaligned global. I could not come up with an example where this happens. FWIW, I tried playing tricks with the linker but it refused to do the wrong thing (-Wl,-sectalign,__DATA,__common,1 ; -Wl,-order_file,insert_pad_byte_to_misalign_global.order_file).
  2. It's easy to declare a global pointer or reference to misaligned data. UBSan already misses this bug: this patch doesn't affect the situation (I filed llvm.org/PR32364 to track the issue).

When you said "it's very easy to misalign a global", did you mean that it's easy for the compiler to mess up? If so, my thinking is that we should write better verifiers, either in our linkers or in LLVM proper. UBSan shouldn't be used as a compiler verifier: if it is, end-users will find its diagnostics to be less actionable. We shouldn't break user bots because of compiler bugs.

If, OTOH, you meant that it's easy for users to declare misaligned globals, I'd appreciate an example.

It's possible to misalign a global definition by misaligning its section with a linker script... but we can probably ignore that possibility.

It's very easy to misalign global declaration, though; for example:

a.c:

extern int a[];
int f(int x) { return a[x]; }

b.c:

char a[] = "asdfzxcv";
vsk added a comment.Mar 21 2017, 5:19 PM

It's possible to misalign a global definition by misaligning its section with a linker script... but we can probably ignore that possibility.

The current implementation does ignore this case.

It's very easy to misalign global declaration, though; for example: [...]

Thanks for pointing this out, I hadn't thought of this case! The current patch does not regress UBSan checking in this area. I'll upload a new test that demonstrates this.

vsk updated this revision to Diff 92572.Mar 21 2017, 5:21 PM

Add a test which shows that we don't regress alignment-checking when accessing extern variables.

efriedma added inline comments.Mar 21 2017, 5:59 PM
test/CodeGenCXX/ubsan-global-alignment.cpp
9 ↗(On Diff #92572)

Probably a good idea to also test extern globals which aren't arrays; arrays are sort of a special-case due to array->pointer decay.

vsk updated this revision to Diff 92732.Mar 22 2017, 4:18 PM

Per Eli's comment: test that we don't regress alignment-checking for extern globals which aren't arrays. I verified that for this case, there is not functional change. However, there *is* a somewhat surprising IR change even at -O0, that is worth calling out.

Before this patch, here's what happens when loading a "long long" from a global struct:

define i64 @load_extern_S1()() #0 {
  br i1 true, label %2, label %1, !prof !2, !nosanitize !3 ;; The frontend appears to be doing some early optimizations here...

; <label>:1:                                      ; preds = %0
  call void @__ubsan_handle_type_mismatch(i8* bitcast ({ { [99 long long i8]*, i32, i32 }, { i16, i16, [12 long long i8] }*, i64, i8 }* @1 to i8*), i64 ptrtoint (%struct.S1* @g_S1 to i64)) #2, !nosanitize !3
  br label %2, !nosanitize !3

; <label>:2:                                      ; preds = %1, %0
  %3 = load i64, i64* getelementptr inbounds (%struct.S1, %struct.S1* @g_S1, i32 0, i32 0), align 8
  ret i64 %3
}

Because this patch skips alignment checking when the base of a MemberExpr is a DeclRefExpr, the IR changes, even though the behavior doesn't:

define i64 @load_extern_S1()() #0 {
  %0 = load i64, i64* getelementptr inbounds (%struct.S1, %struct.S1* @g_S1, i32 0, i32 0), align 8
  ret i64 %0
}

IMO this is an acceptable change which won't lead to missed diagnostics. But I'm calling it out in case anyone thinks otherwise.

Oh, right... constant folding uses the declared alignment of the global to constant-fold the comparison. (I still think it would be interesting to solve, but maybe orthogonal to some extent.)

vsk updated this revision to Diff 95053.Apr 12 2017, 4:27 PM
vsk edited the summary of this revision. (Show Details)

Eli pointed out that it's possible to make alignment checks for extern globals work better (llvm.org/PR32630). One solution depends on emitting alignment checks on bases of member expressions which are DeclRefExprs. I updated this patch so that we'll continue to do that. PTAL.

efriedma accepted this revision.Apr 14 2017, 10:50 AM

LGTM.

test/CodeGenCXX/ubsan-suppress-checks.cpp
25 ↗(On Diff #95053)

LAMBDA-NOT: call void @__ubsan_handle_type_mismatch ?

This revision is now accepted and ready to land.Apr 14 2017, 10:50 AM
vsk added a comment.Apr 14 2017, 3:12 PM

Thanks for the review!

test/CodeGenCXX/ubsan-suppress-checks.cpp
25 ↗(On Diff #95053)

I'll tighten up this test. This method contains two calls to __ubsan_handle_type_mismatch. The first is due to a null+alignment check for "this" at the start of "do_nothing_with_lambda". The second occurs because we have the same checks for the anonymous object backing "f". Before this patch, the first check on "this" did not perform an alignment check. The second check has always been present.

This revision was automatically updated to reflect the committed changes.
cfe/trunk/test/CodeGenCXX/ubsan-suppress-checks.cpp