Index: lib/CodeGen/CGCall.cpp =================================================================== --- lib/CodeGen/CGCall.cpp +++ lib/CodeGen/CGCall.cpp @@ -2906,7 +2906,7 @@ llvm::Instruction *Ret; if (RV) { - EmitReturnValueCheck(RV, EndLoc); + EmitReturnValueCheck(RV); Ret = Builder.CreateRet(RV); } else { Ret = Builder.CreateRetVoid(); @@ -2916,8 +2916,7 @@ Ret->setDebugLoc(std::move(RetDbgLoc)); } -void CodeGenFunction::EmitReturnValueCheck(llvm::Value *RV, - SourceLocation EndLoc) { +void CodeGenFunction::EmitReturnValueCheck(llvm::Value *RV) { // A current decl may not be available when emitting vtable thunks. if (!CurCodeDecl) return; @@ -2950,27 +2949,30 @@ SanitizerScope SanScope(this); - llvm::BasicBlock *Check = nullptr; - llvm::BasicBlock *NoCheck = nullptr; - if (requiresReturnValueNullabilityCheck()) { - // Before doing the nullability check, make sure that the preconditions for - // the check are met. - Check = createBasicBlock("nullcheck"); - NoCheck = createBasicBlock("no.nullcheck"); - Builder.CreateCondBr(RetValNullabilityPrecondition, Check, NoCheck); - EmitBlock(Check); - } + // Make sure the "return" source location is valid. If we're checking a + // nullability annotation, make sure the preconditions for the check are met. + llvm::BasicBlock *Check = createBasicBlock("nullcheck"); + llvm::BasicBlock *NoCheck = createBasicBlock("no.nullcheck"); + llvm::Value *SLocPtr = Builder.CreateLoad(ReturnLocation, "return.sloc.load"); + llvm::Value *CanNullCheck = Builder.CreateIsNotNull(SLocPtr); + if (requiresReturnValueNullabilityCheck()) + CanNullCheck = + Builder.CreateAnd(CanNullCheck, RetValNullabilityPrecondition); + Builder.CreateCondBr(CanNullCheck, Check, NoCheck); + EmitBlock(Check); - // Now do the null check. If the returns_nonnull attribute is present, this - // is done unconditionally. + // Now do the null check. llvm::Value *Cond = Builder.CreateIsNotNull(RV); - llvm::Constant *StaticData[] = { - EmitCheckSourceLocation(EndLoc), EmitCheckSourceLocation(AttrLoc), - }; - EmitCheck(std::make_pair(Cond, CheckKind), Handler, StaticData, None); + llvm::Constant *StaticData[] = {EmitCheckSourceLocation(AttrLoc)}; + llvm::Value *DynamicData[] = {SLocPtr}; + EmitCheck(std::make_pair(Cond, CheckKind), Handler, StaticData, DynamicData); - if (requiresReturnValueNullabilityCheck()) - EmitBlock(NoCheck); + EmitBlock(NoCheck); + +#ifndef NDEBUG + // The return location should not be used after the check has been emitted. + ReturnLocation = Address::invalid(); +#endif } static bool isInAllocaArgument(CGCXXABI &ABI, QualType type) { Index: lib/CodeGen/CGStmt.cpp =================================================================== --- lib/CodeGen/CGStmt.cpp +++ lib/CodeGen/CGStmt.cpp @@ -1024,6 +1024,18 @@ /// if the function returns void, or may be missing one if the function returns /// non-void. Fun stuff :). void CodeGenFunction::EmitReturnStmt(const ReturnStmt &S) { + if (requiresReturnValueCheck()) { + llvm::Constant *SLoc = EmitCheckSourceLocation(S.getLocStart()); + auto *SLocPtr = + new llvm::GlobalVariable(CGM.getModule(), SLoc->getType(), false, + llvm::GlobalVariable::PrivateLinkage, SLoc); + SLocPtr->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global); + CGM.getSanitizerMetadata()->disableSanitizerForGlobal(SLocPtr); + assert(ReturnLocation.isValid() && "No valid return location"); + Builder.CreateStore(Builder.CreateBitCast(SLocPtr, Int8PtrTy), + ReturnLocation); + } + // Returning from an outlined SEH helper is UB, and we already warn on it. if (IsOutlinedSEHHelper) { Builder.CreateUnreachable(); Index: lib/CodeGen/CodeGenFunction.h =================================================================== --- lib/CodeGen/CodeGenFunction.h +++ lib/CodeGen/CodeGenFunction.h @@ -116,9 +116,9 @@ SANITIZER_CHECK(MulOverflow, mul_overflow, 0) \ SANITIZER_CHECK(NegateOverflow, negate_overflow, 0) \ SANITIZER_CHECK(NullabilityArg, nullability_arg, 0) \ - SANITIZER_CHECK(NullabilityReturn, nullability_return, 0) \ + SANITIZER_CHECK(NullabilityReturn, nullability_return, 1) \ SANITIZER_CHECK(NonnullArg, nonnull_arg, 0) \ - SANITIZER_CHECK(NonnullReturn, nonnull_return, 0) \ + SANITIZER_CHECK(NonnullReturn, nonnull_return, 1) \ SANITIZER_CHECK(OutOfBounds, out_of_bounds, 0) \ SANITIZER_CHECK(PointerOverflow, pointer_overflow, 0) \ SANITIZER_CHECK(ShiftOutOfBounds, shift_out_of_bounds, 0) \ @@ -1407,6 +1407,17 @@ return RetValNullabilityPrecondition; } + /// Used to store precise source locations for return statements by the + /// runtime return value checks. + Address ReturnLocation = Address::invalid(); + + /// Check if the return value of this function requires sanitization. + bool requiresReturnValueCheck() const { + return requiresReturnValueNullabilityCheck() || + (SanOpts.has(SanitizerKind::ReturnsNonnullAttribute) && + CurCodeDecl && CurCodeDecl->getAttr()); + } + llvm::BasicBlock *TerminateLandingPad; llvm::BasicBlock *TerminateHandler; llvm::BasicBlock *TrapBB; @@ -1778,7 +1789,7 @@ SourceLocation EndLoc); /// Emit a test that checks if the return value \p RV is nonnull. - void EmitReturnValueCheck(llvm::Value *RV, SourceLocation EndLoc); + void EmitReturnValueCheck(llvm::Value *RV); /// EmitStartEHSpec - Emit the start of the exception spec. void EmitStartEHSpec(const Decl *D); Index: lib/CodeGen/CodeGenFunction.cpp =================================================================== --- lib/CodeGen/CodeGenFunction.cpp +++ lib/CodeGen/CodeGenFunction.cpp @@ -860,6 +860,13 @@ Builder.SetInsertPoint(EntryBB); + // If we're checking the return value, allocate space for a pointer to a + // precise source location of the checked return statement. + if (requiresReturnValueCheck()) { + ReturnLocation = CreateDefaultAlignTempAlloca(Int8PtrTy, "return.sloc.ptr"); + InitTempAlloca(ReturnLocation, llvm::ConstantPointerNull::get(Int8PtrTy)); + } + // Emit subprogram debug descriptor. if (CGDebugInfo *DI = getDebugInfo()) { // Reconstruct the type from the argument list so that implicit parameters, Index: test/CodeGenObjC/ubsan-nonnull-and-nullability.m =================================================================== --- test/CodeGenObjC/ubsan-nonnull-and-nullability.m +++ test/CodeGenObjC/ubsan-nonnull-and-nullability.m @@ -7,16 +7,26 @@ // CHECK-LABEL: define nonnull i32* @f1 __attribute__((returns_nonnull)) int *_Nonnull f1(int *_Nonnull p) { // CHECK: entry: + // CHECK-NEXT: [[SLOC_PTR:%.*]] = alloca i8* // CHECK-NEXT: [[ADDR:%.*]] = alloca i32* + // CHECK-NEXT: store i8* null, i8** [[SLOC_PTR]] // CHECK-NEXT: store i32* [[P:%.*]], i32** [[ADDR]] + // CHECK-NEXT: store {{.*}} [[SLOC_PTR]] // CHECK-NEXT: [[ARG:%.*]] = load i32*, i32** [[ADDR]] + // CHECK-NEXT: [[SLOC:%.*]] = load {{.*}} [[SLOC_PTR]] + // CHECK-NEXT: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC]], null + // CHECK-NEXT: br i1 [[SLOC_NONNULL]], label %nullcheck + // + // CHECK: nullcheck: // CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* [[ARG]], null, !nosanitize // CHECK-NEXT: br i1 [[ICMP]], label %[[CONT:.+]], label %[[HANDLE:[^,]+]] // CHECK: [[HANDLE]]: - // CHECK-NEXT: call void @__ubsan_handle_nonnull_return_abort + // CHECK: call void @__ubsan_handle_nonnull_return // CHECK-NEXT: unreachable, !nosanitize // CHECK: [[CONT]]: - // CHECK-NEXT: ret i32* + // CHECK-NEXT: br label %no.nullcheck + // CHECK: no.nullcheck: + // CHECK-NEXT: ret i32* [[ARG]] return p; } @@ -29,3 +39,23 @@ // CHECK-NOT: call void @__ubsan_handle_nonnull_arg_abort f2((void *)0); } + +// If the return value isn't meant to be checked, make sure we don't check it. +// CHECK-LABEL: define i32* @f3 +int *f3(int *p) { + // CHECK-NOT: return.sloc + // CHECK-NOT: call{{.*}}ubsan + return p; +} + +// Check for a valid "return" source location, even when there is no return +// statement, to avoid accidentally calling the runtime. + +// CHECK-LABEL: define nonnull i32* @f4 +__attribute__((returns_nonnull)) int *f4() { + // CHECK: store i8* null, i8** [[SLOC_PTR:%.*]] + // CHECK: [[SLOC:%.*]] = load {{.*}} [[SLOC_PTR]] + // CHECK: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC]], null + // CHECK: br i1 [[SLOC_NONNULL]], label %nullcheck + // CHECK: nullcheck: +} Index: test/CodeGenObjC/ubsan-nullability.m =================================================================== --- test/CodeGenObjC/ubsan-nullability.m +++ test/CodeGenObjC/ubsan-nullability.m @@ -2,7 +2,7 @@ // RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nullability-arg,nullability-assign,nullability-return -w %s -o - | FileCheck %s // RUN: %clang_cc1 -x objective-c++ -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nullability-arg,nullability-assign,nullability-return -w %s -o - | FileCheck %s -// CHECK: [[NONNULL_RV_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 109, i32 1 {{.*}} i32 100, i32 6 +// CHECK: [[NONNULL_RV_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 100, i32 6 // CHECK: [[NONNULL_ARG_LOC:@.*]] = private unnamed_addr global {{.*}} i32 204, i32 15 {{.*}} i32 190, i32 23 // CHECK: [[NONNULL_ASSIGN1_LOC:@.*]] = private unnamed_addr global {{.*}} i32 305, i32 9 // CHECK: [[NONNULL_ASSIGN2_LOC:@.*]] = private unnamed_addr global {{.*}} i32 405, i32 10 @@ -10,7 +10,7 @@ // CHECK: [[NONNULL_INIT1_LOC:@.*]] = private unnamed_addr global {{.*}} i32 604, i32 25 // CHECK: [[NONNULL_INIT2_LOC1:@.*]] = private unnamed_addr global {{.*}} i32 707, i32 26 // CHECK: [[NONNULL_INIT2_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 707, i32 29 -// CHECK: [[NONNULL_RV_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 817, i32 1 {{.*}} i32 800, i32 6 +// CHECK: [[NONNULL_RV_LOC2:@.*]] = private unnamed_addr global {{.*}} i32 800, i32 6 #define NULL ((void *)0) #define INULL ((int *)NULL) @@ -19,14 +19,11 @@ // CHECK-LABEL: define i32* @{{.*}}nonnull_retval1 #line 100 int *_Nonnull nonnull_retval1(int *p) { - // CHECK: br i1 true, label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize - // CHECK: [[NULL]]: // CHECK: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize - // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize + // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize // CHECK: call void @__ubsan_handle_nullability_return{{.*}}[[NONNULL_RV_LOC1]] return p; - // CHECK: [[NONULL]]: - // CHECK-NEXT: ret i32* + // CHECK: ret i32* } #line 190 @@ -108,10 +105,13 @@ // CHECK-NEXT: [[DO_RV_CHECK_1:%.*]] = and i1 true, [[ARG1CMP]], !nosanitize // CHECK: [[ARG2CMP:%.*]] = icmp ne i32* %arg2, null, !nosanitize // CHECK-NEXT: [[DO_RV_CHECK_2:%.*]] = and i1 [[DO_RV_CHECK_1]], [[ARG2CMP]] - // CHECK: br i1 [[DO_RV_CHECK_2]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize + // CHECK: [[SLOC_PTR:%.*]] = load i8*, i8** %return.sloc.ptr + // CHECK-NEXT: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC_PTR]], null + // CHECK-NEXT: [[DO_RV_CHECK_3:%.*]] = and i1 [[SLOC_NONNULL]], [[DO_RV_CHECK_2]] + // CHECK: br i1 [[DO_RV_CHECK_3]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize // CHECK: [[NULL]]: // CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize - // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize + // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize // CHECK: call void @__ubsan_handle_nullability_return{{.*}}[[NONNULL_RV_LOC2]] return arg1; // CHECK: [[NONULL]]: @@ -129,10 +129,13 @@ +(int *_Nonnull) objc_clsmethod: (int *_Nonnull) arg1 { // CHECK: [[ARG1CMP:%.*]] = icmp ne i32* %arg1, null, !nosanitize // CHECK-NEXT: [[DO_RV_CHECK:%.*]] = and i1 true, [[ARG1CMP]] - // CHECK: br i1 [[DO_RV_CHECK]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize + // CHECK: [[SLOC_PTR:%.*]] = load i8*, i8** %return.sloc.ptr + // CHECK-NEXT: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC_PTR]], null + // CHECK-NEXT: [[DO_RV_CHECK_2:%.*]] = and i1 [[SLOC_NONNULL]], [[DO_RV_CHECK]] + // CHECK: br i1 [[DO_RV_CHECK_2]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize // CHECK: [[NULL]]: // CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize - // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize + // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize // CHECK: call void @__ubsan_handle_nullability_return{{.*}} return arg1; // CHECK: [[NONULL]]: @@ -143,10 +146,13 @@ -(int *_Nonnull) objc_method: (int *_Nonnull) arg1 { // CHECK: [[ARG1CMP:%.*]] = icmp ne i32* %arg1, null, !nosanitize // CHECK-NEXT: [[DO_RV_CHECK:%.*]] = and i1 true, [[ARG1CMP]] - // CHECK: br i1 [[DO_RV_CHECK]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize + // CHECK: [[SLOC_PTR:%.*]] = load i8*, i8** %return.sloc.ptr + // CHECK-NEXT: [[SLOC_NONNULL:%.*]] = icmp ne i8* [[SLOC_PTR]], null + // CHECK-NEXT: [[DO_RV_CHECK_2:%.*]] = and i1 [[SLOC_NONNULL]], [[DO_RV_CHECK]] + // CHECK: br i1 [[DO_RV_CHECK_2]], label %[[NULL:.*]], label %[[NONULL:.*]], !nosanitize // CHECK: [[NULL]]: // CHECK-NEXT: [[ICMP:%.*]] = icmp ne i32* {{.*}}, null, !nosanitize - // CHECK-NEXT: br i1 [[ICMP]], {{.*}}, !nosanitize + // CHECK: br i1 [[ICMP]], {{.*}}, !nosanitize // CHECK: call void @__ubsan_handle_nullability_return{{.*}} return arg1; // CHECK: [[NONULL]]: