Ensure bitmap for ternary condition is updated before visiting children

[evodius96]

This is a fix for MC/DC issue #78453 in which a ConditionalOperator that evaluates a complex condition was incorrectly updating its global bitmap after visiting its LHS and RHS children. This was wrong because if the LHS or RHS also evaluate a complex condition, the MCDC temporary bitmap value will get corrupted. The fix is to ensure that the bitmap is updated prior to visiting the LHS and RHS.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Alan Phipps (evodius96)

Changes

This is a fix for MC/DC issue #78453 in which a ConditionalOperator that evaluates a complex condition was incorrectly updating its global bitmap after visiting its LHS and RHS children. This was wrong because if the LHS or RHS also evaluate a complex condition, the MCDC temporary bitmap value will get corrupted. The fix is to ensure that the bitmap is updated prior to visiting the LHS and RHS.

---

Full diff: https://github.com/llvm/llvm-project/pull/78814.diff

2 Files Affected:

• (modified) clang/lib/CodeGen/CGExprScalar.cpp (+14-4)
• (added) clang/test/Profile/c-mcdc-logicalop-ternary.c (+81)

diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 9ec185153d12b1..181b15e9c7d0a7 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -4960,6 +4960,13 @@ VisitAbstractConditionalOperator(const AbstractConditionalOperator *E) {
                            CGF.getProfileCount(lhsExpr));
 
   CGF.EmitBlock(LHSBlock);
+
+  // If the top of the logical operator nest, update the MCDC bitmap for the
+  // ConditionalOperator prior to visiting its LHS and RHS blocks, since they
+  // may also contain a boolean expression.
+  if (CGF.MCDCLogOpStack.empty())
+    CGF.maybeUpdateMCDCTestVectorBitmap(condExpr);
+
   CGF.incrementProfileCounter(E);
   eval.begin(CGF);
   Value *LHS = Visit(lhsExpr);
@@ -4969,6 +4976,13 @@ VisitAbstractConditionalOperator(const AbstractConditionalOperator *E) {
   Builder.CreateBr(ContBlock);
 
   CGF.EmitBlock(RHSBlock);
+
+  // If the top of the logical operator nest, update the MCDC bitmap for the
+  // ConditionalOperator prior to visiting its LHS and RHS blocks, since they
+  // may also contain a boolean expression.
+  if (CGF.MCDCLogOpStack.empty())
+    CGF.maybeUpdateMCDCTestVectorBitmap(condExpr);
+
   eval.begin(CGF);
   Value *RHS = Visit(rhsExpr);
   eval.end(CGF);
@@ -4987,10 +5001,6 @@ VisitAbstractConditionalOperator(const AbstractConditionalOperator *E) {
   PN->addIncoming(LHS, LHSBlock);
   PN->addIncoming(RHS, RHSBlock);
 
-  // If the top of the logical operator nest, update the MCDC bitmap.
-  if (CGF.MCDCLogOpStack.empty())
-    CGF.maybeUpdateMCDCTestVectorBitmap(condExpr);
-
   return PN;
 }
 
diff --git a/clang/test/Profile/c-mcdc-logicalop-ternary.c b/clang/test/Profile/c-mcdc-logicalop-ternary.c
new file mode 100644
index 00000000000000..558643f422021c
--- /dev/null
+++ b/clang/test/Profile/c-mcdc-logicalop-ternary.c
@@ -0,0 +1,81 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple %s -o - -emit-llvm -fprofile-instrument=clang -fcoverage-mapping -fcoverage-mcdc | FileCheck %s -check-prefix=MCDC
+// RUN: %clang_cc1 -triple %itanium_abi_triple %s -o - -emit-llvm -fprofile-instrument=clang -fcoverage-mapping | FileCheck %s -check-prefix=NOMCDC
+
+int test(int a, int b, int c, int d, int e, int f) {
+  return ((a || b) ? (c && d) : (e || f));
+}
+
+// NOMCDC-NOT: %mcdc.addr
+// NOMCDC-NOT: __profbm_test
+
+// MCDC BOOKKEEPING.
+// MCDC: @__profbm_test = private global [3 x i8] zeroinitializer
+
+// ALLOCATE MCDC TEMP AND ZERO IT.
+// MCDC-LABEL: @test(
+// MCDC: %mcdc.addr = alloca i32, align 4
+// MCDC: store i32 0, ptr %mcdc.addr, align 4
+
+// TERNARY TRUE SHOULD UPDATE THE BITMAP WITH RESULT AT ELEMENT 0.
+// MCDC-LABEL: cond.true:
+// MCDC-DAG:  %[[TEMP:mcdc.temp[0-9]*]] = load i32, ptr %mcdc.addr, align 4
+// MCDC:  %[[LAB1:[0-9]+]] = lshr i32 %[[TEMP]], 3
+// MCDC:  %[[LAB2:[0-9]+]] = zext i32 %[[LAB1]] to i64
+// MCDC:  %[[LAB3:[0-9]+]] = add i64 ptrtoint (ptr @__profbm_test to i64), %[[LAB2]]
+// MCDC:  %[[LAB4:[0-9]+]] = inttoptr i64 %[[LAB3]] to ptr
+// MCDC:  %[[LAB5:[0-9]+]] = and i32 %[[TEMP]], 7
+// MCDC:  %[[LAB6:[0-9]+]] = trunc i32 %[[LAB5]] to i8
+// MCDC:  %[[LAB7:[0-9]+]] = shl i8 1, %[[LAB6]]
+// MCDC:  %[[LAB8:mcdc.bits[0-9]*]] = load i8, ptr %[[LAB4]], align 1
+// MCDC:  %[[LAB9:[0-9]+]] = or i8 %[[LAB8]], %[[LAB7]]
+// MCDC:  store i8 %[[LAB9]], ptr %[[LAB4]], align 1
+
+// CHECK FOR ZERO OF MCDC TEMP
+// MCDC: store i32 0, ptr %mcdc.addr, align 4
+
+// TERNARY TRUE YIELDS TERNARY LHS LOGICAL-AND.
+// TERNARY LHS LOGICAL-AND SHOULD UPDATE THE BITMAP WITH RESULT AT ELEMENT 1.
+// MCDC-LABEL: land.end:
+// MCDC-DAG:  %[[TEMP:mcdc.temp[0-9]*]] = load i32, ptr %mcdc.addr, align 4
+// MCDC:  %[[LAB1:[0-9]+]] = lshr i32 %[[TEMP]], 3
+// MCDC:  %[[LAB2:[0-9]+]] = zext i32 %[[LAB1]] to i64
+// MCDC:  %[[LAB3:[0-9]+]] = add i64 ptrtoint (ptr getelementptr inbounds ([3 x i8], ptr @__profbm_test, i32 0, i32 1) to i64), %[[LAB2]]
+// MCDC:  %[[LAB4:[0-9]+]] = inttoptr i64 %[[LAB3]] to ptr
+// MCDC:  %[[LAB5:[0-9]+]] = and i32 %[[TEMP]], 7
+// MCDC:  %[[LAB6:[0-9]+]] = trunc i32 %[[LAB5]] to i8
+// MCDC:  %[[LAB7:[0-9]+]] = shl i8 1, %[[LAB6]]
+// MCDC:  %[[LAB8:mcdc.bits[0-9]*]] = load i8, ptr %[[LAB4]], align 1
+// MCDC:  %[[LAB9:[0-9]+]] = or i8 %[[LAB8]], %[[LAB7]]
+// MCDC:  store i8 %[[LAB9]], ptr %[[LAB4]], align 1
+
+// TERNARY FALSE SHOULD UPDATE THE BITMAP WITH RESULT AT ELEMENT 0.
+// MCDC-LABEL: cond.false:
+// MCDC-DAG:  %[[TEMP:mcdc.temp[0-9]*]] = load i32, ptr %mcdc.addr, align 4
+// MCDC:  %[[LAB1:[0-9]+]] = lshr i32 %[[TEMP]], 3
+// MCDC:  %[[LAB2:[0-9]+]] = zext i32 %[[LAB1]] to i64
+// MCDC:  %[[LAB3:[0-9]+]] = add i64 ptrtoint (ptr @__profbm_test to i64), %[[LAB2]]
+// MCDC:  %[[LAB4:[0-9]+]] = inttoptr i64 %[[LAB3]] to ptr
+// MCDC:  %[[LAB5:[0-9]+]] = and i32 %[[TEMP]], 7
+// MCDC:  %[[LAB6:[0-9]+]] = trunc i32 %[[LAB5]] to i8
+// MCDC:  %[[LAB7:[0-9]+]] = shl i8 1, %[[LAB6]]
+// MCDC:  %[[LAB8:mcdc.bits[0-9]*]] = load i8, ptr %[[LAB4]], align 1
+// MCDC:  %[[LAB9:[0-9]+]] = or i8 %[[LAB8]], %[[LAB7]]
+// MCDC:  store i8 %[[LAB9]], ptr %[[LAB4]], align 1
+
+// CHECK FOR ZERO OF MCDC TEMP
+// MCDC: store i32 0, ptr %mcdc.addr, align 4
+
+// TERNARY FALSE YIELDS TERNARY RHS LOGICAL-OR.
+// TERNARY RHS LOGICAL-OR SHOULD UPDATE THE BITMAP WITH RESULT AT ELEMENT 2.
+// MCDC-LABEL: lor.end:
+// MCDC-DAG:  %[[TEMP:mcdc.temp[0-9]*]] = load i32, ptr %mcdc.addr, align 4
+// MCDC:  %[[LAB1:[0-9]+]] = lshr i32 %[[TEMP]], 3
+// MCDC:  %[[LAB2:[0-9]+]] = zext i32 %[[LAB1]] to i64
+// MCDC:  %[[LAB3:[0-9]+]] = add i64 ptrtoint (ptr getelementptr inbounds ([3 x i8], ptr @__profbm_test, i32 0, i32 2) to i64), %[[LAB2]]
+// MCDC:  %[[LAB4:[0-9]+]] = inttoptr i64 %[[LAB3]] to ptr
+// MCDC:  %[[LAB5:[0-9]+]] = and i32 %[[TEMP]], 7
+// MCDC:  %[[LAB6:[0-9]+]] = trunc i32 %[[LAB5]] to i8
+// MCDC:  %[[LAB7:[0-9]+]] = shl i8 1, %[[LAB6]]
+// MCDC:  %[[LAB8:mcdc.bits[0-9]*]] = load i8, ptr %[[LAB4]], align 1
+// MCDC:  %[[LAB9:[0-9]+]] = or i8 %[[LAB8]], %[[LAB7]]
+// MCDC:  store i8 %[[LAB9]], ptr %[[LAB4]], align 1

[chapuni]

Looks good and works for me :)

In the future, could we consider to generalize splitting MC/DC expression groups in "including known and safe operators", not only "cutting unsafe" operators? I guess we could resolve "unknown nested conditions" as "individual expressions".