[CodeGen] Fix some broken IR generated by -fsanitize=unsigned-integer-overflow

[erik.pilkington]

Summary

I think the author of the function assumed that GetInsertBlock() wouldn't change from where atomicPHI was created, but this isn't true when -fsanitize=unsigned-integer-overflow is enabled (we generate an overflow/continuation label). Fix by keeping track of the block we want to return to to complete the cmpxchg loop.

rdar://48406558

Thanks!

[erik.pilkington]

Use FileCheck in the test, NFC.

[ahatanak]

clang/lib/CodeGen/CGExprScalar.cpp

2921 ↗

(On Diff #188631)

Would passing atomicPHI->getParent() instead of opBB to Builder.CreateCondBr fix the crash?

clang/test/CodeGen/sanitize-atomic-int-overflow.c

3 ↗

(On Diff #188631)

It's probably better to add some check strings here.

3 ↗

(On Diff #188631)

I see this has been fixed in the updated patch.

[erik.pilkington]

clang/lib/CodeGen/CGExprScalar.cpp

2921 ↗

(On Diff #188631)

Yeah, atomicOpBB and atomicPHI->getParent() should always be equal here. I'll update the patch to use this, since there is one less variable to keep track of.

[erik.pilkington]

Use atomicPHI->getParent() instead of tracking the block.

[ahatanak]

LGTM

cfe/trunk/lib/CodeGen/CGExprScalar.cpp

// Objective-C pointer types.
     else
       value = CGF.EmitCheckedInBoundsGEP(value, sizeValue,
                                          /*SignedIndices=*/false, isSubtraction,
                                          E->getExprLoc(), "incdec.objptr");
     value = Builder.CreateBitCast(value, input->getType());
   }
 
   if (atomicPHI) {
-    llvm::BasicBlock *opBB = Builder.GetInsertBlock();
+    llvm::BasicBlock *curBlock = Builder.GetInsertBlock();
     llvm::BasicBlock *contBB = CGF.createBasicBlock("atomic_cont", CGF.CurFn);
     auto Pair = CGF.EmitAtomicCompareExchange(
         LV, RValue::get(atomicPHI), RValue::get(value), E->getExprLoc());
     llvm::Value *old = CGF.EmitToMemory(Pair.first.getScalarVal(), type);
     llvm::Value *success = Pair.second;
-    atomicPHI->addIncoming(old, opBB);
-    Builder.CreateCondBr(success, contBB, opBB);
+    atomicPHI->addIncoming(old, curBlock);
+    Builder.CreateCondBr(success, contBB, atomicPHI->getParent());
     Builder.SetInsertPoint(contBB);
     return isPre ? value : input;
   }
 
   // Store the updated result through the lvalue.
   if (LV.isBitField())
     CGF.EmitStoreThroughBitfieldLValue(RValue::get(value), LV, &value);
   else
LValue ScalarExprEmitter::EmitCompoundAssignLValue(
   Result = (this->*Func)(OpInfo);
 
   // Convert the result back to the LHS type,
   // potentially with Implicit Conversion sanitizer check.
   Result = EmitScalarConversion(Result, E->getComputationResultType(), LHSTy,
                                 Loc, ScalarConversionOpts(CGF.SanOpts));
 
   if (atomicPHI) {
-    llvm::BasicBlock *opBB = Builder.GetInsertBlock();
+    llvm::BasicBlock *curBlock = Builder.GetInsertBlock();
     llvm::BasicBlock *contBB = CGF.createBasicBlock("atomic_cont", CGF.CurFn);
     auto Pair = CGF.EmitAtomicCompareExchange(
         LHSLV, RValue::get(atomicPHI), RValue::get(Result), E->getExprLoc());
     llvm::Value *old = CGF.EmitToMemory(Pair.first.getScalarVal(), LHSTy);
     llvm::Value *success = Pair.second;
-    atomicPHI->addIncoming(old, opBB);
-    Builder.CreateCondBr(success, contBB, opBB);
+    atomicPHI->addIncoming(old, curBlock);
+    Builder.CreateCondBr(success, contBB, atomicPHI->getParent());
     Builder.SetInsertPoint(contBB);
     return LHSLV;
   }
 
   // Store the result value into the LHS lvalue. Bit-fields are handled
   // specially because the result is altered by the store, i.e., [C99 6.5.16p1]
   // 'An assignment expression has the value of the left operand after the
   // assignment...'.

cfe/trunk/test/CodeGen/sanitize-atomic-int-overflow.c

+// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 -fsanitize=unsigned-integer-overflow %s -emit-llvm -o - | FileCheck %s
+
+_Atomic(unsigned) atomic;
+
+// CHECK-LABEL: define void @cmpd_assign
+void cmpd_assign() {
+  // CHECK: br label %[[LOOP_START:.*]]
+
+  // CHECK: [[LOOP_START]]:
+  // CHECK-NEXT: phi i32 {{.*}}, [ {{.*}}, %[[INCOMING_BLOCK:.*]] ]
+
+  // CHECK: [[INCOMING_BLOCK]]:
+  // CHECK-NEXT: cmpxchg
+  // CHECK-NEXT: extractvalue
+  // CHECK-NEXT: extractvalue
+  // CHECK-NEXT: br i1 %8, label %{{.*}}, label %[[LOOP_START]]
+  atomic += 1;
+}
+
+// CHECK-LABEL: define void @inc
+void inc() {
+  // CHECK: br label %[[LOOP_START:.*]]
+
+  // CHECK: [[LOOP_START]]:
+  // CHECK-NEXT: phi i32 {{.*}}, [ {{.*}}, %[[INCOMING_BLOCK:.*]] ]
+
+  // CHECK: [[INCOMING_BLOCK]]:
+  // CHECK-NEXT: cmpxchg
+  // CHECK-NEXT: extractvalue
+  // CHECK-NEXT: extractvalue
+  // CHECK-NEXT: br i1 %8, label %{{.*}}, label %[[LOOP_START]]
+  atomic++;
+}