[clang] Ensure bitmap for ternary condition is updated before visiting children (PR #78814)
Alan Phipps via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 19 16:09:52 PST 2024
https://github.com/evodius96 created https://github.com/llvm/llvm-project/pull/78814
This is a fix for MC/DC issue https://github.com/llvm/llvm-project/issues/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.
>From cbd5a8caaac0fd3d1dc9d4090d00e5bece6a41cf Mon Sep 17 00:00:00 2001
From: Alan Phipps <a-phipps at ti.com>
Date: Fri, 19 Jan 2024 17:54:25 -0600
Subject: [PATCH] Ensure bitmap for ternary condition is updated prior to visit
of LHS and RHS
This is a fix for MC/DC issue https://github.com/llvm/llvm-project/issues/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.
---
clang/lib/CodeGen/CGExprScalar.cpp | 18 ++++-
clang/test/Profile/c-mcdc-logicalop-ternary.c | 81 +++++++++++++++++++
2 files changed, 95 insertions(+), 4 deletions(-)
create mode 100644 clang/test/Profile/c-mcdc-logicalop-ternary.c
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
More information about the cfe-commits
mailing list