This revision was automatically updated to reflect the committed changes.
Closed by commit rL300370: [ubsan] Reduce alignment checking of C++ object
pointers (authored by vedantk).
Changed prior to commit:
https://reviews.llvm.org/D30283?vs=95053=95352#toc
Repository:
rL LLVM
vsk added a comment.
Thanks for the review!
Comment at: test/CodeGenCXX/ubsan-suppress-checks.cpp:25
+// LAMBDA: and i64 %[[THISINT2]], 3, !nosanitize
+// LAMBDA: call void @__ubsan_handle_type_mismatch
+
efriedma wrote:
> LAMBDA-NOT: call void
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.
LGTM.
Comment at: test/CodeGenCXX/ubsan-suppress-checks.cpp:25
+// LAMBDA: and i64 %[[THISINT2]], 3, !nosanitize
+// LAMBDA: call void
vsk updated this revision to Diff 95053.
vsk edited the summary of this revision.
vsk added a comment.
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
efriedma added a comment.
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.)
https://reviews.llvm.org/D30283
vsk updated this revision to Diff 92732.
vsk added a comment.
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,
efriedma added inline comments.
Comment at: test/CodeGenCXX/ubsan-global-alignment.cpp:9
+extern S1 S1_array[];
+extern S1 *S1ptr_array[];
+
Probably a good idea to also test extern globals which aren't arrays; arrays
are sort of a special-case due to
vsk updated this revision to Diff 92572.
vsk added a comment.
Add a test which shows that we don't regress alignment-checking when accessing
extern variables.
https://reviews.llvm.org/D30283
Files:
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprCXX.cpp
lib/CodeGen/CodeGenFunction.cpp
vsk added a comment.
In https://reviews.llvm.org/D30283#707007, @efriedma wrote:
> 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
efriedma added a comment.
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]; }
vsk added a comment.
Hi Eli, thanks for your feedback :).
In https://reviews.llvm.org/D30283#702085, @efriedma wrote:
> 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
efriedma added a comment.
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.
https://reviews.llvm.org/D30283
vsk added a reviewer: rsmith.
vsk added a comment.
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.
https://reviews.llvm.org/D30283
___
vsk created this revision.
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
14 matches
Mail list logo
