This revision was automatically updated to reflect the committed changes.
Closed by commit rL295391: [ubsan] Reduce null checking of C++ object pointers
(PR27581) (authored by vedantk).
Changed prior to commit:
https://reviews.llvm.org/D29530?vs=88259=88808#toc
Repository:
rL LLVM
arphaman accepted this revision.
arphaman added a comment.
This revision is now accepted and ready to land.
LGTM
https://reviews.llvm.org/D29530
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
vsk updated this revision to Diff 88259.
vsk marked an inline comment as done.
vsk added a comment.
- Tighten up the tests per Alex's suggestion.
https://reviews.llvm.org/D29530
Files:
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprCXX.cpp
lib/CodeGen/CodeGenFunction.cpp
vsk marked 2 inline comments as done.
vsk added inline comments.
Comment at: test/CodeGenCXX/ubsan-suppress-null-checks.cpp:8
+ int load_member() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
arphaman added inline comments.
Comment at: test/CodeGenCXX/ubsan-suppress-null-checks.cpp:8
+ int load_member() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
I think this kind of check
vsk updated this revision to Diff 88075.
vsk edited the summary of this revision.
vsk added a reviewer: arphaman.
vsk added a comment.
- Check 'this' once per method. This supports the partial sanitization use-case.
- Flesh out the predicate for avoiding null checks on object pointers. I
vsk added a comment.
Ah, I did miss ParenExpr. Maybe it would be better to use
Expr::isImplicitCXXThis, since it handles this case and some more. Later, we
can go back and see if it's feasible to handle static/const casts in
isImplicitCXXThis to catch more cases.
In
arphaman added a comment.
Btw, you mentioned that 'this' must have been null-checked before the method is
called. But what if it's called from some part of code that was compiled
without `-fsanitize=null`? Wouldn't we still want at least one check to see if
'this' is null in a method?
arphaman added a comment.
Thanks!
I guess for the sake of completeness it might be useful for handle 'this' in
parens as well, since it could up in macros:
#define MEMBER(x) (x)->y
...
MEMBER(this)
...
Btw, I was curious if we could do a similar optimization in Objective-C, but
vsk updated this revision to Diff 87334.
vsk edited the summary of this revision.
vsk added a comment.
@arphaman thank you for the feedback!
Per Alex's comments:
- Add test for explicit use of the 'this' pointer.
- Handle cases where the 'this' pointer may be casted (implicitly, or
otherwise).
arphaman added a comment.
I feel like you should have at least one statement in the test that uses
explicit 'this' to access a field/method.
Does your patch handle fields/methods in base classes as well? I think 'this'
might be implicitly converted in the base of the member expression.
Also,
vsk created this revision.
Given a load of a member variable from an instance method ('this->x'),
ubsan inserts a null check for 'this', and another null check for
'>x', before allowing the load to occur. Both of these checks are
redundant, because 'this' must have been null-checked before the
12 matches
Mail list logo
