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-phi...@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
