Retry^2: [ubsan] Reduce null checking of C++ object pointers (PR27581)
This patch teaches ubsan to insert exactly one null check for the 'this'
pointer per method/lambda.
Previously, given a load of a member variable from an instance method
('this->x'), ubsan would insert a null check for 'this', and another
null check for '&this->x', before allowing the load to occur.
Similarly, given a call to a method from another method bound to the
same instance ('this->foo()'), ubsan would 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.
Testing: check-clang, check-ubsan, and a stage2 ubsan build.
I also compiled X86FastISel.cpp with -fsanitize=null using
patched/unpatched clangs based on r293572. Here are the number of null
checks emitted:
-------------------------------------
| Setup | # of null checks |
-------------------------------------
| unpatched, -O0 | 21767 |
| patched, -O0 | 10758 |
-------------------------------------
Changes since the initial commit:
- Don't introduce any unintentional object-size or alignment checks.
- Don't rely on IRGen of C labels in the test.
Differential Revision: https://reviews.llvm.org/D29530
llvm-svn: 295515
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 8661251..276716f 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -952,15 +952,46 @@
E->getType());
}
+bool CodeGenFunction::CanElideObjectPointerNullCheck(const Expr *Obj) {
+ if (isa<DeclRefExpr>(Obj))
+ return true;
+
+ const Expr *Base = Obj;
+ while (!isa<CXXThisExpr>(Base)) {
+ // The result of a dynamic_cast can be null.
+ if (isa<CXXDynamicCastExpr>(Base))
+ return false;
+
+ if (const auto *CE = dyn_cast<CastExpr>(Base)) {
+ Base = CE->getSubExpr();
+ } else if (const auto *PE = dyn_cast<ParenExpr>(Base)) {
+ Base = PE->getSubExpr();
+ } else if (const auto *UO = dyn_cast<UnaryOperator>(Base)) {
+ if (UO->getOpcode() == UO_Extension)
+ Base = UO->getSubExpr();
+ else
+ return false;
+ } else {
+ return false;
+ }
+ }
+ return true;
+}
+
LValue CodeGenFunction::EmitCheckedLValue(const Expr *E, TypeCheckKind TCK) {
LValue LV;
if (SanOpts.has(SanitizerKind::ArrayBounds) && isa<ArraySubscriptExpr>(E))
LV = EmitArraySubscriptExpr(cast<ArraySubscriptExpr>(E), /*Accessed*/true);
else
LV = EmitLValue(E);
- if (!isa<DeclRefExpr>(E) && !LV.isBitField() && LV.isSimple())
+ if (!isa<DeclRefExpr>(E) && !LV.isBitField() && LV.isSimple()) {
+ SanitizerSet SkippedChecks;
+ if (const auto *ME = dyn_cast<MemberExpr>(E))
+ if (CanElideObjectPointerNullCheck(ME->getBase()))
+ SkippedChecks.set(SanitizerKind::Null, true);
EmitTypeCheck(TCK, E->getExprLoc(), LV.getPointer(),
- E->getType(), LV.getAlignment());
+ E->getType(), LV.getAlignment(), SkippedChecks);
+ }
return LV;
}
@@ -3340,7 +3371,11 @@
AlignmentSource AlignSource;
Address Addr = EmitPointerWithAlignment(BaseExpr, &AlignSource);
QualType PtrTy = BaseExpr->getType()->getPointeeType();
- EmitTypeCheck(TCK_MemberAccess, E->getExprLoc(), Addr.getPointer(), PtrTy);
+ SanitizerSet SkippedChecks;
+ if (CanElideObjectPointerNullCheck(BaseExpr))
+ SkippedChecks.set(SanitizerKind::Null, true);
+ EmitTypeCheck(TCK_MemberAccess, E->getExprLoc(), Addr.getPointer(), PtrTy,
+ /*Alignment=*/CharUnits::Zero(), SkippedChecks);
BaseLV = MakeAddrLValue(Addr, PtrTy, AlignSource);
} else
BaseLV = EmitCheckedLValue(BaseExpr, TCK_MemberAccess);
diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp
index ebe0841..3751ef1 100644
--- a/clang/lib/CodeGen/CGExprCXX.cpp
+++ b/clang/lib/CodeGen/CGExprCXX.cpp
@@ -290,10 +290,15 @@
if (CE)
CallLoc = CE->getExprLoc();
- EmitTypeCheck(isa<CXXConstructorDecl>(CalleeDecl)
- ? CodeGenFunction::TCK_ConstructorCall
- : CodeGenFunction::TCK_MemberCall,
- CallLoc, This.getPointer(), C.getRecordType(CalleeDecl->getParent()));
+ SanitizerSet SkippedChecks;
+ if (const auto *CMCE = dyn_cast<CXXMemberCallExpr>(CE))
+ if (CanElideObjectPointerNullCheck(CMCE->getImplicitObjectArgument()))
+ SkippedChecks.set(SanitizerKind::Null, true);
+ EmitTypeCheck(
+ isa<CXXConstructorDecl>(CalleeDecl) ? CodeGenFunction::TCK_ConstructorCall
+ : CodeGenFunction::TCK_MemberCall,
+ CallLoc, This.getPointer(), C.getRecordType(CalleeDecl->getParent()),
+ /*Alignment=*/CharUnits::Zero(), SkippedChecks);
// FIXME: Uses of 'MD' past this point need to be audited. We may need to use
// 'CalleeDecl' instead.
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 00d5b5f..1ebfd79 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -948,6 +948,15 @@
// fast register allocator would be happier...
CXXThisValue = CXXABIThisValue;
}
+
+ // Null-check the 'this' pointer once per function, if it's available.
+ if (CXXThisValue) {
+ SanitizerSet SkippedChecks;
+ SkippedChecks.set(SanitizerKind::Alignment, true);
+ SkippedChecks.set(SanitizerKind::ObjectSize, true);
+ EmitTypeCheck(TCK_Load, Loc, CXXThisValue, MD->getThisType(getContext()),
+ /*Alignment=*/CharUnits::Zero(), SkippedChecks);
+ }
}
// If any of the arguments have a variably modified type, make sure to
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 09f1763..80a9a0e 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -2030,6 +2030,9 @@
llvm::BlockAddress *GetAddrOfLabel(const LabelDecl *L);
llvm::BasicBlock *GetIndirectGotoBlock();
+ /// Check if the null check for \p ObjectPointer can be skipped.
+ static bool CanElideObjectPointerNullCheck(const Expr *ObjectPointer);
+
/// EmitNullInitialization - Generate code to set a value of the given type to
/// null, If the type contains data member pointers, they will be initialized
/// to -1 in accordance with the Itanium C++ ABI.