[ubsan] Improve diagnostics for return value checks (clang)

[vsk]

Summary

This patch makes ubsan's nonnull return value diagnostics more precise,
which makes the diagnostics more useful when there are multiple return
statements in a function. Example:

1 |__attribute__((returns_nonnull)) char *foo() {
2 |  if (...) {
3 |    return expr_which_might_evaluate_to_null();
4 |  } else {
5 |    return another_expr_which_might_evaluate_to_null();
6 |  }
7 |} // <- The current diagnostic always points here!

runtime error: Null returned from Line 7, Column 2!

With this patch, the diagnostic would point to either Line 3, Column 5
or Line 5, Column 5.

This is done by emitting source location metadata for each return
statement in a sanitized function. The runtime is passed a pointer to
the appropriate metadata so that it can prepare and deduplicate reports.

Compiler-rt patch (with more tests): https://reviews.llvm.org/D34298

[arphaman]

It looks like if we have a function without the return (like the sample below), we will pass in a 0 as the location pointer. This will prevent a report of a runtime error as your compiler-rt change ignores the location pointers that are nil. Is this a bug or is this the intended behaviour?

int *_Nonnull nonnull_retval1(int *p) {
}

[filcab]

Splitting the attrloc from the useloc might make sense since we would be able to emit attrloc just once. But I don't see why we need to store/load those pointers in runtime instead of just caching the Constant* in CodeGenFunction.
I'd also like to have some asserts and explicit resets to nullptr after use on the ReturnLocation variable, if possible.

lib/CodeGen/CGStmt.cpp

1035

Can't you just keep the Constant* around and use it later for the static data? Instead of creating a global var and have runtime store/load?

lib/CodeGen/CodeGenFunction.h

1412

Maybe CurrentReturnLocation?

[vsk]

In D34299#787795, @arphaman wrote:

It looks like if we have a function without the return (like the sample below), we will pass in a 0 as the location pointer. This will prevent a report of a runtime error as your compiler-rt change ignores the location pointers that are nil. Is this a bug or is this the intended behaviour?

int *_Nonnull nonnull_retval1(int *p) {
}

This is the intended behavior (I'll add a test). Users should not see a "null return value" diagnostic here. There is another check, -fsanitize=return, which can catch this issue.

@filcab --

Splitting the attrloc from the useloc might make sense since we would be able to emit attrloc just once. But I don't see why we need to store/load those pointers in runtime instead of just caching the Constant* in CodeGenFunction.

The source locations aren't constants. The ubsan runtime uses a bit inside of source location structures as a flag. When an issue is diagnosed at a particular source location, that bit is atomically set. This is how ubsan implements issue deduplication.

I'd also like to have some asserts and explicit resets to nullptr after use on the ReturnLocation variable, if possible.

Resetting Address fields in CodeGenFunction doesn't appear to be an established practice. Could you explain what this would be in aid of?

[arphaman]

In D34299#788152, @vsk wrote:

In D34299#787795, @arphaman wrote:

It looks like if we have a function without the return (like the sample below), we will pass in a 0 as the location pointer. This will prevent a report of a runtime error as your compiler-rt change ignores the location pointers that are nil. Is this a bug or is this the intended behaviour?

int *_Nonnull nonnull_retval1(int *p) {
}

This is the intended behavior (I'll add a test). Users should not see a "null return value" diagnostic here. There is another check, -fsanitize=return, which can catch this issue.

Ok. However, when the source is not using C++, the check for -fsanitize=return won't be emitted. So we will end up with a call to __ubsan_handle_nullability_return_abort without a location, which won't report a diagnostic, but the program will crash because compiler-rt will call abort. This seems like a regression, since previously in C mode this diagnostic was reported.

@filcab --

Splitting the attrloc from the useloc might make sense since we would be able to emit attrloc just once. But I don't see why we need to store/load those pointers in runtime instead of just caching the Constant* in CodeGenFunction.

The source locations aren't constants. The ubsan runtime uses a bit inside of source location structures as a flag. When an issue is diagnosed at a particular source location, that bit is atomically set. This is how ubsan implements issue deduplication.

I'd also like to have some asserts and explicit resets to nullptr after use on the ReturnLocation variable, if possible.

Resetting Address fields in CodeGenFunction doesn't appear to be an established practice. Could you explain what this would be in aid of?

[filcab]

In D34299#788152, @vsk wrote:

The source locations aren't constants. The ubsan runtime uses a bit inside of source location structures as a flag. When an issue is diagnosed at a particular source location, that bit is atomically set. This is how ubsan implements issue deduplication.

It's still an llvm::Constant. Just like in StaticData, in line 2966.
Basically, I don't see why we need to add the store/load and an additional indirection, since the pointer is constant, and we can just emit the static data as before.
We're already doing Data->Loc.acquire(); for the current version (and all the other checks).

I'd also like to have some asserts and explicit resets to nullptr after use on the ReturnLocation variable, if possible.

Resetting Address fields in CodeGenFunction doesn't appear to be an established practice. Could you explain what this would be in aid of?

It would be a sanity check and help with code reading/keeping in mind the lifetime of the information. I'm ok with that happening only on !NDEBUG builds.

Reading the code, I don't know if a ReturnLocation might end up being used for more than one checks. If it's supposed to be a different one per check, etc.
If it's only supposed to be used once, I'd rather set it to nullptr right after use (at least when !NDEBUG), and assert(!ReturnLocation) before setting. It's not a big deal, but would help with making sense of the flow of information when debugging.

[vsk]

In D34299#788379, @filcab wrote:

In D34299#788152, @vsk wrote:

The source locations aren't constants. The ubsan runtime uses a bit inside of source location structures as a flag. When an issue is diagnosed at a particular source location, that bit is atomically set. This is how ubsan implements issue deduplication.

It's still an llvm::Constant. Just like in StaticData, in line 2966.
Basically, I don't see why we need to add the store/load and an additional indirection, since the pointer is constant, and we can just emit the static data as before.

My earlier response made the assumption that you wanted a copy of the source location passed to the runtime handler, by-value. I see now that you're wondering why the source locations aren't part of the static data structure. That's because the location of the return statement isn't known at compile time, i.e it's not static data. The location depends on which return statement is executed.

We're already doing Data->Loc.acquire(); for the current version (and all the other checks).

The other checks do not allow the source location within a static data object to change.

I'd also like to have some asserts and explicit resets to nullptr after use on the ReturnLocation variable, if possible.

Resetting Address fields in CodeGenFunction doesn't appear to be an established practice. Could you explain what this would be in aid of?

It would be a sanity check and help with code reading/keeping in mind the lifetime of the information. I'm ok with that happening only on !NDEBUG builds.

Reading the code, I don't know if a ReturnLocation might end up being used for more than one checks. If it's supposed to be a different one per check, etc.
If it's only supposed to be used once, I'd rather set it to nullptr right after use (at least when !NDEBUG), and assert(!ReturnLocation) before setting. It's not a big deal, but would help with making sense of the flow of information when debugging.

Sure, I'll reset it to Address::invalid().

[vsk]

Handle functions without return statements correctly (fixing an issue pointed out by @arphaman). Now, the instrumentation always checks that we have a valid return location before calling the runtime. I added tests for this on the clang side: we can't test it on the compiler-rt side, because functions without return statements can cause stack corruption / crashes on Darwin.

lib/CodeGen/CGStmt.cpp

1035

I hope I've cleared this up, but: we need to store the source location constant _somewhere_, before we emit the return value check. That's because we can't infer which return location to use at compile time.

lib/CodeGen/CodeGenFunction.h

1412

I'd prefer to keep it the way it is, for consistency with the "ReturnValue" field.

[arphaman]

Ok, so now the null check return.sloc.load won't call the checker in compiler-rt and so the program won't abort and won't hit the unreachable. I have one question tough:

This patch changes the behavior of this sanitizer for the example that I gave above. Previously a runtime diagnostic was emitted, but now there is none. While I'm not saying that the previous behaviour was correct, I'm wondering if the new behaviour is right. I think that for C++ it makes sense, but I don't know the right answer for C. I'm leaning more towards the new behaviour, since technically in C falling off without returning a value is not UB unless that return value is used by the caller. But at the same time, since we don't diagnose return UB for C, maybe it's still worth diagnosing this particular issue? The user might not catch it otherwise at all (or they might catch it later when they try to access it, but by that point they might not know where the pointer came from). WDYT?

[vsk]

In D34299#788908, @arphaman wrote:

Ok, so now the null check return.sloc.load won't call the checker in compiler-rt and so the program won't abort and won't hit the unreachable. I have one question tough:

This patch changes the behavior of this sanitizer for the example that I gave above.

Yes, in the case where there is no explicit return, and the return value happens to be null.

Previously a runtime diagnostic was emitted, but now there is none. While I'm not saying that the previous behaviour was correct, I'm wondering if the new behaviour is right. I think that for C++ it makes sense, but I don't know the right answer for C. I'm leaning more towards the new behaviour, since technically in C falling off without returning a value is not UB unless that return value is used by the caller. But at the same time, since we don't diagnose return UB for C, maybe it's still worth diagnosing this particular issue? The user might not catch it otherwise at all (or they might catch it later when they try to access it, but by that point they might not know where the pointer came from). WDYT?

Without seeing what the caller does with the result, the return value check can't do a good job of diagnosing missing returns. I think clang should emit a diagnostic in the example above, and -Wreturn-type does. So I think the new behavior is appropriate.

[arphaman]

Fair enough. LGTM from my side.

[filcab]

In D34299#788427, @vsk wrote:

I hope I've cleared this up, but: we need to store the source location constant _somewhere_, before we emit the return value check. That's because we can't infer which return location to use at compile time.

Yes, sorry about that. I was thinking about the code the wrong way around. I'm ok with this patch and how it got in. Sorry for the delay in replying.

lib/CodeGen/CGCall.cpp

#endif
   }
 
   case ABIArgInfo::Expand:
     llvm_unreachable("Invalid ABI kind for return argument");
   }
 
   llvm::Instruction *Ret;
   if (RV) {
-    EmitReturnValueCheck(RV, EndLoc);
+    EmitReturnValueCheck(RV);
     Ret = Builder.CreateRet(RV);
   } else {
     Ret = Builder.CreateRetVoid();
   }
 
   if (RetDbgLoc)
     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;
 
   ReturnsNonNullAttr *RetNNAttr = nullptr;
   if (SanOpts.has(SanitizerKind::ReturnsNonnullAttribute))
     RetNNAttr = CurCodeDecl->getAttr<ReturnsNonNullAttr>();
 
if (requiresReturnValueNullabilityCheck()) {
     NoCheck = createBasicBlock("no.nullcheck");
     Builder.CreateCondBr(RetValNullabilityPrecondition, Check, NoCheck);
     EmitBlock(Check);
   }
 
   // Now do the null check. If the returns_nonnull attribute is present, this
   // is done unconditionally.
   llvm::Value *Cond = Builder.CreateIsNotNull(RV);
-  llvm::Constant *StaticData[] = {
-      EmitCheckSourceLocation(EndLoc), EmitCheckSourceLocation(AttrLoc),
+  llvm::Constant *StaticData[] = {EmitCheckSourceLocation(AttrLoc)};
+  llvm::Value *DynamicData[] = {
+    Builder.CreateLoad(ReturnLocation, "return.sloc.load")
   };
-  EmitCheck(std::make_pair(Cond, CheckKind), Handler, StaticData, None);
+  EmitCheck(std::make_pair(Cond, CheckKind), Handler, StaticData, DynamicData);
 
   if (requiresReturnValueNullabilityCheck())
     EmitBlock(NoCheck);
 }
 
 static bool isInAllocaArgument(CGCXXABI &ABI, QualType type) {
   const CXXRecordDecl *RD = type->getAsCXXRecordDecl();
   return RD && ABI.getRecordArgABI(RD) == CGCXXABI::RAA_DirectInMemory;

lib/CodeGen/CGStmt.cpp

void CodeGenFunction::EmitReturnOfRValue(RValue RV, QualType Ty) {
   }
   EmitBranchThroughCleanup(ReturnBlock);
 }
 
 /// EmitReturnStmt - Note that due to GCC extensions, this can have an operand
 /// 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();
     Builder.ClearInsertionPoint();
   }
 
   // Emit the result value, even if unused, to evalute the side effects.
   const Expr *RV = S.getRetValue();

lib/CodeGen/CodeGenFunction.h

#define LIST_SANITIZER_CHECKS                                                  \
   SANITIZER_CHECK(DynamicTypeCacheMiss, dynamic_type_cache_miss, 0)            \
   SANITIZER_CHECK(FloatCastOverflow, float_cast_overflow, 0)                   \
   SANITIZER_CHECK(FunctionTypeMismatch, function_type_mismatch, 0)             \
   SANITIZER_CHECK(LoadInvalidValue, load_invalid_value, 0)                     \
   SANITIZER_CHECK(MissingReturn, missing_return, 0)                            \
   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)                    \
   SANITIZER_CHECK(SubOverflow, sub_overflow, 0)                                \
   SANITIZER_CHECK(TypeMismatch, type_mismatch, 1)                              \
   SANITIZER_CHECK(VLABoundNotPositive, vla_bound_not_positive, 0)
 
 enum SanitizerHandler {
private:
   llvm::Value *RetValNullabilityPrecondition = nullptr;
 
   /// Check if -fsanitize=nullability-return instrumentation is required for
   /// this function.
   bool requiresReturnValueNullabilityCheck() const {
     return RetValNullabilityPrecondition;
   }
 
+  /// Used by the various return statement sanitizers to select a precise
+  /// source location for diagnostics.
+  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<ReturnsNonNullAttr>());
+  }
+
   llvm::BasicBlock *TerminateLandingPad;
   llvm::BasicBlock *TerminateHandler;
   llvm::BasicBlock *TrapBB;
 
   /// True if we need emit the life-time markers.
   const bool ShouldEmitLifetimeMarkers;
 
   /// Add OpenCL kernel arg metadata and the kernel attribute meatadata to
void EmitFunctionProlog(const CGFunctionInfo &FI,
                           const FunctionArgList &Args);
 
   /// EmitFunctionEpilog - Emit the target specific LLVM code to return the
   /// given temporary.
   void EmitFunctionEpilog(const CGFunctionInfo &FI, bool EmitRetDbgLoc,
                           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);
 
   /// EmitEndEHSpec - Emit the end of the exception spec.
   void EmitEndEHSpec(const Decl *D);
 
   /// getTerminateLandingPad - Return a landing pad that just calls terminate.

lib/CodeGen/CodeGenFunction.cpp

void CodeGenFunction::StartFunction(GlobalDecl GD,
   // folded.
   llvm::Value *Undef = llvm::UndefValue::get(Int32Ty);
   AllocaInsertPt = new llvm::BitCastInst(Undef, Int32Ty, "allocapt", EntryBB);
 
   ReturnBlock = getJumpDestInCurrentScope("return");
 
   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()) {
+    auto ReturnSLocPtr =
+        Builder.CreateAlloca(Int8PtrTy, nullptr, "return.sloc.ptr");
+    unsigned Alignment = CGM.getDataLayout().getTypeAllocSize(Int8PtrTy);
+    ReturnSLocPtr->setAlignment(Alignment);
+    ReturnLocation = Address(ReturnSLocPtr, CharUnits::fromQuantity(Alignment));
+    Builder.CreateStore(llvm::ConstantPointerNull::get(Int8PtrTy),
+                        ReturnLocation);
+  }
+
   // Emit subprogram debug descriptor.
   if (CGDebugInfo *DI = getDebugInfo()) {
     // Reconstruct the type from the argument list so that implicit parameters,
     // such as 'this' and 'vtt', show up in the debug info. Preserve the calling
     // convention.
     CallingConv CC = CallingConv::CC_C;
     if (auto *FD = dyn_cast_or_null<FunctionDecl>(D))
       if (const auto *SrcFnTy = FD->getType()->getAs<FunctionType>())

test/CodeGenObjC/ubsan-nonnull-and-nullability.m

 // REQUIRES: asserts
 // RUN: %clang_cc1 -x objective-c -emit-llvm -triple x86_64-apple-macosx10.10.0 -fsanitize=nullability-return,returns-nonnull-attribute,nullability-arg,nonnull-attribute %s -o - -w | FileCheck %s
 
 // If both the annotation and the attribute are present, prefer the attribute,
 // since it actually affects IRGen.
 
 // CHECK-LABEL: define nonnull i32* @f1
 __attribute__((returns_nonnull)) int *_Nonnull f1(int *_Nonnull p) {
   // CHECK: entry:
   // CHECK-NEXT: [[ADDR:%.*]] = alloca i32*
+  // CHECK-NEXT: [[SLOC_PTR:%.*]] = alloca i8*
+  // 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: [[ICMP:%.*]] = icmp ne i32* [[ARG]], null, !nosanitize
+  // CHECK-NEXT: load {{.*}} [[SLOC_PTR]]
   // 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*
   return p;
 }
 
 // CHECK-LABEL: define void @f2
 void f2(int *_Nonnull __attribute__((nonnull)) p) {}
 
 // CHECK-LABEL: define void @call_f2
 void call_f2() {
   // CHECK: call void @__ubsan_handle_nonnull_arg_abort
   // 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;
+}

test/CodeGenObjC/ubsan-nullability.m

 // REQUIRES: asserts
 // 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
 // CHECK: [[NONNULL_ASSIGN3_LOC:@.*]] = private unnamed_addr global {{.*}} i32 506, i32 10
 // 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)
 #define INNULL ((int *_Nonnull)NULL)
 
 // 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*
 }
 
 #line 190
 void nonnull_arg(int *_Nonnull p) {}
int *_Nonnull nonnull_retval2(int *_Nonnull arg1,  //< Test this.
                               int arg5, ...) {
   // CHECK: [[ARG1CMP:%.*]] = icmp ne i32* %arg1, null, !nosanitize
   // 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: [[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]]:
   // CHECK-NEXT: ret i32*
 }
 
 @interface A
 +(int *_Nonnull) objc_clsmethod: (int *_Nonnull) arg1;
 -(int *_Nonnull) objc_method: (int *_Nonnull) arg1;
 @end
 
 @implementation A
 
 // CHECK-LABEL: define internal i32* @"\01+[A objc_clsmethod:]"
 +(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: [[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]]:
   // CHECK-NEXT: ret i32*
 }
 
 // CHECK-LABEL: define internal i32* @"\01-[A objc_method:]"
 -(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: [[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]]:
   // CHECK-NEXT: ret i32*
 }
 @end
 
 // CHECK-LABEL: define void @{{.*}}call_A