[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)
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
https://reviews.llvm.org/D29530
Files:
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/lib/CodeGen/CGExprCXX.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.h
cfe/trunk/test/CodeGen/catch-undef-behavior.c
cfe/trunk/test/CodeGen/sanitize-recover.c
cfe/trunk/test/CodeGenCXX/ubsan-suppress-null-checks.cpp
Index: cfe/trunk/test/CodeGenCXX/ubsan-suppress-null-checks.cpp
===
--- cfe/trunk/test/CodeGenCXX/ubsan-suppress-null-checks.cpp
+++ cfe/trunk/test/CodeGenCXX/ubsan-suppress-null-checks.cpp
@@ -0,0 +1,187 @@
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=null | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=null -DCHECK_LAMBDA | FileCheck %s --check-prefix=LAMBDA
+
+struct A {
+ int foo;
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A10do_nothingEv
+ void do_nothing() {
+// CHECK: icmp ne %struct.A* %[[THIS1:[a-z0-9]+]], null, !nosanitize
+// CHECK: ptrtoint %struct.A* %[[THIS1]] to i64, !nosanitize
+// CHECK-NEXT: call void @__ubsan_handle_type_mismatch
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+// CHECK: ret void
+ }
+
+#ifdef CHECK_LAMBDA
+ // LAMBDA-LABEL: define linkonce_odr void @_ZN1A22do_nothing_with_lambdaEv
+ void do_nothing_with_lambda() {
+// LAMBDA: icmp ne %struct.A* %[[THIS2:[a-z0-9]+]], null, !nosanitize
+// LAMBDA: ptrtoint %struct.A* %[[THIS2]] to i64, !nosanitize
+// LAMBDA-NEXT: call void @__ubsan_handle_type_mismatch
+
+auto f = [&] {
+ foo = 0;
+};
+f();
+
+// LAMBDA: ret void
+ }
+
+// Check the IR for the lambda:
+//
+// LAMBDA-LABEL: define linkonce_odr void @_ZZN1A22do_nothing_with_lambdaEvENKUlvE_clEv
+// LAMBDA: call void @__ubsan_handle_type_mismatch
+// LAMBDA-NOT: call void @__ubsan_handle_type_mismatch
+// LAMBDA: ret void
+#endif
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A11load_memberEv
+ int load_member() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK: L1
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+L1:
+return foo;
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A11call_methodEv
+ int call_method() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK: L2
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+L2:
+return load_member();
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A15assign_member_1Ev
+ void assign_member_1() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK: L3
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+L3:
+foo = 0;
+// CHECK: ret void
+ }
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A15assign_member_2Ev
+ void assign_member_2() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK: L4
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+L4:
+(__extension__ (this))->foo = 0;
+// CHECK: ret void
+ }
+
+ // CHECK-LABEL: define linkonce_odr void @_ZNK1A15assign_member_3Ev
+ void assign_member_3() const {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK: L5
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+L5:
+const_cast(this)->foo = 0;
+// CHECK: ret void
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A22call_through_referenceERS_
+ static int call_through_reference(A ) {
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+return a.load_member();
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A20call_through_pointerEPS_
+ static int call_through_pointer(A *a) {
+// CHECK: call void @__ubsan_handle_type_mismatch
+return a->load_member();
+// CHECK: ret i32
+ }
+};
+
+struct B {
+ operator A*() const { return nullptr; }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1B11load_memberEv
+ static int load_member() {
+// Null-check before converting it to an A*.
+// CHECK: call void @__ubsan_handle_type_mismatch
+//
+// Null-check the result of the conversion before using it.
+// CHECK: call void @__ubsan_handle_type_mismatch
+//
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+B b;
+return static_cast(b)->load_member();
+// CHECK: ret i32
+ }
+};
+
+struct Base {
+ int foo;
+
+ virtual int load_member_1() = 0;
+};
+
+struct Derived : public Base {
+ int bar;
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN7Derived13load_member_2Ev
+ int load_member_2() {
+// CHECK: call void
[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)
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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)
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
lib/CodeGen/CodeGenFunction.h
test/CodeGen/catch-undef-behavior.c
test/CodeGen/sanitize-recover.c
test/CodeGenCXX/ubsan-suppress-null-checks.cpp
Index: test/CodeGenCXX/ubsan-suppress-null-checks.cpp
===
--- /dev/null
+++ test/CodeGenCXX/ubsan-suppress-null-checks.cpp
@@ -0,0 +1,161 @@
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=null | FileCheck %s
+
+struct A {
+ int foo;
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A10do_nothingEv
+ void do_nothing() {
+// CHECK: icmp ne %struct.A* %[[THIS1:[a-z0-9]+]], null, !nosanitize
+// CHECK: ptrtoint %struct.A* %[[THIS1]] to i64, !nosanitize
+// CHECK-NEXT: call void @__ubsan_handle_type_mismatch
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A11load_memberEv
+ int load_member() {
+// CHECK: icmp ne %struct.A* %[[THIS2:[a-z0-9]+]], null, !nosanitize
+// CHECK: ptrtoint %struct.A* %[[THIS2]] to i64, !nosanitize
+// CHECK-NEXT: call void @__ubsan_handle_type_mismatch
+// CHECK: L1
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+L1:
+return foo;
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A11call_methodEv
+ int call_method() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK: L2
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+L2:
+return load_member();
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A15assign_member_1Ev
+ void assign_member_1() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK: L3
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+L3:
+foo = 0;
+// CHECK: ret void
+ }
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A15assign_member_2Ev
+ void assign_member_2() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK: L4
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+L4:
+(__extension__ (this))->foo = 0;
+// CHECK: ret void
+ }
+
+ // CHECK-LABEL: define linkonce_odr void @_ZNK1A15assign_member_3Ev
+ void assign_member_3() const {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK: L5
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+L5:
+const_cast(this)->foo = 0;
+// CHECK: ret void
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A22call_through_referenceERS_
+ static int call_through_reference(A ) {
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+return a.load_member();
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A20call_through_pointerEPS_
+ static int call_through_pointer(A *a) {
+// CHECK: call void @__ubsan_handle_type_mismatch
+return a->load_member();
+// CHECK: ret i32
+ }
+};
+
+struct B {
+ operator A*() const { return nullptr; }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1B11load_memberEv
+ static int load_member() {
+// Null-check before converting it to an A*.
+// CHECK: call void @__ubsan_handle_type_mismatch
+//
+// Null-check the result of the conversion before using it.
+// CHECK: call void @__ubsan_handle_type_mismatch
+//
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+B b;
+return static_cast(b)->load_member();
+// CHECK: ret i32
+ }
+};
+
+struct Base {
+ int foo;
+
+ virtual int load_member_1() = 0;
+};
+
+struct Derived : public Base {
+ int bar;
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN7Derived13load_member_2Ev
+ int load_member_2() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK: L6
+L6:
+// Null-check the result of the cast before using it.
+// CHECK: call void @__ubsan_handle_type_mismatch
+//
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+return dynamic_cast(this)->load_member_1();
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN7Derived13load_member_3Ev
+ int load_member_3() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK: L7
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+L7:
+return reinterpret_cast(static_cast(this))->foo;
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN7Derived13load_member_1Ev
+ int load_member_1() override {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK: L8
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+L8:
+return foo + bar;
+// CHECK: ret i32
+ }
+};
+
+void force_irgen() {
+ A *a;
+ a->do_nothing();
+ a->load_member();
+ a->call_method();
[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)
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 wrote:
> I think this kind of check would've passed even when this patch is not
> applied. I understand that you need the first check for the single 'this'
> check, but it should be rewritten to ensure that it couldn't be the check for
> the `return foo`. Maybe you could use a C label before return and then check
> that the 'this' check is performed before the label?
Yes, this check should be tighter. This just checks that the number of checks
in the method goes from 2 to 1. The label idea is great.
https://reviews.llvm.org/D29530
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)
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 would've passed even when this patch is not applied.
I understand that you need the first check for the single 'this' check, but it
should be rewritten to ensure that it couldn't be the check for the `return
foo`. Maybe you could use a C label before return and then check that the
'this' check is performed before the label?
https://reviews.llvm.org/D29530
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)
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
couldn't use isImplicitCXXThis like I'd wanted, because we'd like to catch
explicit this exprs as well.
https://reviews.llvm.org/D29530
Files:
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprCXX.cpp
lib/CodeGen/CodeGenFunction.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGen/catch-undef-behavior.c
test/CodeGen/sanitize-recover.c
test/CodeGenCXX/ubsan-suppress-null-checks.cpp
Index: test/CodeGenCXX/ubsan-suppress-null-checks.cpp
===
--- /dev/null
+++ test/CodeGenCXX/ubsan-suppress-null-checks.cpp
@@ -0,0 +1,135 @@
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=null | FileCheck %s
+
+struct A {
+ int foo;
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A11load_memberEv
+ int load_member() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+return foo;
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A11call_methodEv
+ int call_method() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+return load_member();
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A15assign_member_1Ev
+ void assign_member_1() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+foo = 0;
+// CHECK: ret void
+ }
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A15assign_member_2Ev
+ void assign_member_2() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+(__extension__ (this))->foo = 0;
+// CHECK: ret void
+ }
+
+ // CHECK-LABEL: define linkonce_odr void @_ZNK1A15assign_member_3Ev
+ void assign_member_3() const {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+const_cast(this)->foo = 0;
+// CHECK: ret void
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A22call_through_referenceERS_
+ static int call_through_reference(A ) {
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+return a.load_member();
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A20call_through_pointerEPS_
+ static int call_through_pointer(A *a) {
+// CHECK: call void @__ubsan_handle_type_mismatch
+return a->load_member();
+// CHECK: ret i32
+ }
+};
+
+struct B {
+ operator A*() const { return nullptr; }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1B11load_memberEv
+ static int load_member() {
+// Null-check before converting it to an A*.
+// CHECK: call void @__ubsan_handle_type_mismatch
+//
+// Null-check the result of the conversion before using it.
+// CHECK: call void @__ubsan_handle_type_mismatch
+//
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+B b;
+return static_cast(b)->load_member();
+// CHECK: ret i32
+ }
+};
+
+struct Base {
+ int foo;
+
+ virtual int load_member_1() = 0;
+};
+
+struct Derived : public Base {
+ int bar;
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN7Derived13load_member_2Ev
+ int load_member_2() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+//
+// Null-check the result of the cast before using it.
+// CHECK: call void @__ubsan_handle_type_mismatch
+//
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+return dynamic_cast(this)->load_member_1();
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN7Derived13load_member_3Ev
+ int load_member_3() {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+return reinterpret_cast(static_cast(this))->foo;
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN7Derived13load_member_1Ev
+ int load_member_1() override {
+// CHECK: call void @__ubsan_handle_type_mismatch
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+return foo + bar;
+// CHECK: ret i32
+ }
+};
+
+void force_irgen() {
+ A *a;
+ a->load_member();
+ a->call_method();
+ a->assign_member_1();
+ a->assign_member_2();
+ a->assign_member_3();
+ A::call_through_reference(*a);
+ A::call_through_pointer(a);
+
+ B::load_member();
+
+ Base *b = new Derived;
+ b->load_member_1();
+
+ Derived *d;
+ d->load_member_2();
+ d->load_member_3();
+}
Index: test/CodeGen/sanitize-recover.c
===
--- test/CodeGen/sanitize-recover.c
+++
[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)
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 https://reviews.llvm.org/D29530#671816, @arphaman wrote: > 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? That's fair, I think that would address the concerns about the partial sanitization use case brought up in the original PR. https://reviews.llvm.org/D29530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)
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? https://reviews.llvm.org/D29530 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)
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
'self' can be set to null there inside the body of a method. I guess maybe it
would be possible to avoid the null check on self if we can prove it wasn't
modified, but that's out of scope of this patch.
Comment at: lib/CodeGen/CGExprCXX.cpp:294
+ bool SkipNullCheck = false;
+ if (const auto *CMCE = dyn_cast(CE)) {
+SkipNullCheck =
You can avoid the '{' '}' here.
https://reviews.llvm.org/D29530
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)
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).
We're able to eliminate more null checks with this version of the patch. See
the updated patch summary for the X86FastISel.cpp numbers.
https://reviews.llvm.org/D29530
Files:
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprCXX.cpp
lib/CodeGen/CodeGenFunction.h
test/CodeGen/catch-undef-behavior.c
test/CodeGen/sanitize-recover.c
test/CodeGenCXX/ubsan-suppress-null-checks.cpp
Index: test/CodeGenCXX/ubsan-suppress-null-checks.cpp
===
--- /dev/null
+++ test/CodeGenCXX/ubsan-suppress-null-checks.cpp
@@ -0,0 +1,126 @@
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=null | FileCheck %s
+
+struct A {
+ int foo;
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A11load_memberEv
+ int load_member() {
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+return foo;
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A11call_methodEv
+ int call_method() {
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+return load_member();
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A15assign_member_1Ev
+ void assign_member_1() {
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+foo = 0;
+// CHECK: ret void
+ }
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A15assign_member_2Ev
+ void assign_member_2() {
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+this->foo = 0;
+// CHECK: ret void
+ }
+
+ // CHECK-LABEL: define linkonce_odr void @_ZNK1A15assign_member_3Ev
+ void assign_member_3() const {
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+const_cast(this)->foo = 0;
+// CHECK: ret void
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A22call_through_referenceERS_
+ static int call_through_reference(A ) {
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+return a.load_member();
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A20call_through_pointerEPS_
+ static int call_through_pointer(A *a) {
+// CHECK: call void @__ubsan_handle_type_mismatch
+return a->load_member();
+// CHECK: ret i32
+ }
+};
+
+struct B {
+ operator A*() const { return nullptr; }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1B11load_memberEv
+ static int load_member() {
+// Null-check before converting it to an A*.
+// CHECK: call void @__ubsan_handle_type_mismatch
+//
+// Null-check the result of the conversion before using it.
+// CHECK: call void @__ubsan_handle_type_mismatch
+//
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+B b;
+return static_cast(b)->load_member();
+// CHECK: ret i32
+ }
+};
+
+struct Base {
+ int foo;
+
+ virtual int load_member_1() = 0;
+};
+
+struct Derived : public Base {
+ int bar;
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN7Derived13load_member_2Ev
+ int load_member_2() {
+// Null-check the result of the cast before using it.
+// CHECK: call void @__ubsan_handle_type_mismatch
+//
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+return dynamic_cast(this)->load_member_1();
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN7Derived13load_member_3Ev
+ int load_member_3() {
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+return reinterpret_cast(static_cast(this))->foo;
+// CHECK: ret i32
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN7Derived13load_member_1Ev
+ int load_member_1() override {
+// CHECK-NOT: call void @__ubsan_handle_type_mismatch
+return foo + bar;
+// CHECK: ret i32
+ }
+};
+
+void force_irgen() {
+ A *a;
+ a->load_member();
+ a->call_method();
+ a->assign_member_1();
+ a->assign_member_2();
+ a->assign_member_3();
+ A::call_through_reference(*a);
+ A::call_through_pointer(a);
+
+ B::load_member();
+
+ Base *b = new Derived;
+ b->load_member_1();
+
+ Derived *d;
+ d->load_member_2();
+ d->load_member_3();
+}
Index: test/CodeGen/sanitize-recover.c
===
--- test/CodeGen/sanitize-recover.c
+++ test/CodeGen/sanitize-recover.c
@@ -19,20 +19,17 @@
void foo() {
union { int i; } u;
u.i=1;
- // PARTIAL: %[[CHECK0:.*]] = icmp ne {{.*}}* %[[PTR:.*]], null
-
// PARTIAL: %[[SIZE:.*]] = call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false)
- // PARTIAL-NEXT: %[[CHECK1:.*]] = icmp uge i64 %[[SIZE]], 4
+ // PARTIAL-NEXT: %[[CHECK0:.*]] = icmp uge i64 %[[SIZE]], 4
// PARTIAL: %[[MISALIGN:.*]] = and i64 {{.*}}, 3
- // PARTIAL-NEXT:
[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)
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, what about cases like this one:
struct A {
int x;
void methodNotConst() const {
const_cast(this)->x = 0;
}
};
https://reviews.llvm.org/D29530
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29530: [ubsan] Reduce null checking of C++ object pointers (PR27581)
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 method
is called.
Similarly, given a call to a method from another method bound to the
same instance ('this->foo()'), ubsan inserts a redundant null check for
'this'. There is also a redundant null check in the case where the
object pointer is a reference ('Ref.foo()').
This patch teaches ubsan to remove the redundant null checks identified
above. I'm not sure I've gone about this in the way suggested in PR27581,
and would appreciate any advice/corrections.
Testing: check-clang and check-ubsan. I also compiled X86FastISel.cpp
with -fsanitize=null using patched/unpatched clangs based on r293572.
Here are the number of null checks emitted in various setups:
| Setup | # of null checks |
| unpatched, -O0 | 21767|
| unpatched, -O3 | 11862|
| patched, -O0 | 13178|
| patched, -O3 | 5547 |
https://reviews.llvm.org/D29530
Files:
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprCXX.cpp
test/CodeGenCXX/ubsan-suppress-null-checks.cpp
Index: test/CodeGenCXX/ubsan-suppress-null-checks.cpp
===
--- /dev/null
+++ test/CodeGenCXX/ubsan-suppress-null-checks.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin10 -emit-llvm -o - %s
-fsanitize=null | FileCheck %s
+
+struct A {
+ int foo;
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A5func1Ev
+ int func1() {
+// CHECK-NOT: ubsan_handler_type_mismatch
+return foo;
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A5func2Ev
+ int func2() {
+// CHECK-NOT: ubsan_handler_type_mismatch
+return func1();
+ }
+
+ // CHECK-LABEL: define linkonce_odr void @_ZN1A5func3Ev
+ void func3() {
+// CHECK-NOT: ubsan_handler_type_mismatch
+foo = 0;
+ }
+
+ // CHECK-LABEL: define linkonce_odr i32 @_ZN1A5func4ERS_
+ static int func4(A ) {
+// CHECK-NOT: ubsan_handler_type_mismatch
+return a.func1();
+ }
+};
+
+// Force clang to emit IR for A's methods.
+void bar() {
+ A *a;
+ a->func1();
+ a->func2();
+ a->func3();
+ A::func4(*a);
+}
Index: lib/CodeGen/CGExprCXX.cpp
===
--- lib/CodeGen/CGExprCXX.cpp
+++ lib/CodeGen/CGExprCXX.cpp
@@ -290,10 +290,16 @@
if (CE)
CallLoc = CE->getExprLoc();
- EmitTypeCheck(isa(CalleeDecl)
-? CodeGenFunction::TCK_ConstructorCall
-: CodeGenFunction::TCK_MemberCall,
-CallLoc, This.getPointer(),
C.getRecordType(CalleeDecl->getParent()));
+ bool SkipNullCheck = false;
+ if (const auto *CMCE = dyn_cast(CE)) {
+Expr *IOA = CMCE->getImplicitObjectArgument();
+SkipNullCheck = isa(IOA) || isa(IOA);
+ }
+ EmitTypeCheck(
+ isa(CalleeDecl) ?
CodeGenFunction::TCK_ConstructorCall
+ : CodeGenFunction::TCK_MemberCall,
+ CallLoc, This.getPointer(), C.getRecordType(CalleeDecl->getParent()),
+ /*Alignment=*/CharUnits::Zero(), SkipNullCheck);
// FIXME: Uses of 'MD' past this point need to be audited. We may need to use
// 'CalleeDecl' instead.
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -953,9 +953,13 @@
LV = EmitArraySubscriptExpr(cast(E), /*Accessed*/true);
else
LV = EmitLValue(E);
- if (!isa(E) && !LV.isBitField() && LV.isSimple())
+ if (!isa(E) && !LV.isBitField() && LV.isSimple()) {
+bool SkipNullCheck = false;
+if (const auto *ME = dyn_cast(E))
+ SkipNullCheck = isa(ME->getBase());
EmitTypeCheck(TCK, E->getExprLoc(), LV.getPointer(),
- E->getType(), LV.getAlignment());
+ E->getType(), LV.getAlignment(), SkipNullCheck);
+ }
return LV;
}
@@ -3335,7 +3339,9 @@
AlignmentSource AlignSource;
Address Addr = EmitPointerWithAlignment(BaseExpr, );
QualType PtrTy = BaseExpr->getType()->getPointeeType();
-EmitTypeCheck(TCK_MemberAccess, E->getExprLoc(), Addr.getPointer(), PtrTy);
+bool SkipNullCheck = isa(BaseExpr);
+EmitTypeCheck(TCK_MemberAccess, E->getExprLoc(), Addr.getPointer(), PtrTy,
+ /*Alignment=*/CharUnits::Zero(), SkipNullCheck);
BaseLV = MakeAddrLValue(Addr, PtrTy, AlignSource);
} else
BaseLV = EmitCheckedLValue(BaseExpr, TCK_MemberAccess);
Index: test/CodeGenCXX/ubsan-suppress-null-checks.cpp
===
--- /dev/null
+++ test/CodeGenCXX/ubsan-suppress-null-checks.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin10 -emit-llvm -o - %s -fsanitize=null | FileCheck
