https://github.com/jvoung updated https://github.com/llvm/llvm-project/pull/168994
>From ed5acde1d7e9070c576b921ec0b04f97ea10f3d7 Mon Sep 17 00:00:00 2001 From: Jan Voung <[email protected]> Date: Fri, 21 Nov 2025 03:44:05 +0000 Subject: [PATCH 1/2] [clang][dataflow] Handle more glvalue cases of the ConditionalOperator transfer In the dataflow framework, the builtin transfer function currently only handles the GLValue result case of ConditionalOperator when the true and false expression StorageLocations are exactly the same. Ideally / we have wanted to introduce alias sets to handle when the Locs are different. However, that is a larger change to the framework (and we may need to introduce weak updates). For now, do something simpler to at least handle when the GLValue is immediately cast to an RValue, by making up a distinct StorageLocation that holds the join of the true and false expression values (when not a record). This seems like the most common case, so seems worth covering. The case when an LValue is needed and can be updated later (and thus needs a link to the original storage locations) seems more rare, and we currently do not handle such updates either, so this intermediate step is no different (for that case). --- clang/lib/Analysis/FlowSensitive/Transfer.cpp | 23 ++- .../Analysis/FlowSensitive/TransferTest.cpp | 131 ++++++++++++++++++ 2 files changed, 153 insertions(+), 1 deletion(-) diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp index 06f12784aa82d..28c7fe3a5e1bc 100644 --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -769,8 +769,29 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> { StorageLocation *TrueLoc = TrueEnv->getStorageLocation(*S->getTrueExpr()); StorageLocation *FalseLoc = FalseEnv->getStorageLocation(*S->getFalseExpr()); - if (TrueLoc == FalseLoc && TrueLoc != nullptr) + if (TrueLoc == FalseLoc && TrueLoc != nullptr) { Env.setStorageLocation(*S, *TrueLoc); + } else if (!S->getType()->isRecordType()) { + // Ideally, we would have something like an "alias set" to say that the + // result StorageLocation can be either of the locations from the + // TrueEnv or FalseEnv. Then, when this ConditionalOperator is + // (a) used in an LValueToRValue cast, the value is the join of all of + // the values in the alias set. + // (b) or, used in an assignment to the resulting LValue, the assignment + // *may* update all of the locations in the alias set. + // For now, we do the simpler thing of creating a new StorageLocation + // and joining the values right away, handling only case (a). + // Otherwise, the dataflow framework needs to be updated be able to + // represent alias sets and weak updates (for the "may"). + StorageLocation &Loc = Env.createStorageLocation(*S); + Env.setStorageLocation(*S, Loc); + if (Value *Val = Environment::joinValues( + S->getType(), TrueEnv->getValue(*S->getTrueExpr()), *TrueEnv, + FalseEnv->getValue(*S->getFalseExpr()), *FalseEnv, Env, + Model)) { + Env.setValue(Loc, *Val); + } + } } else if (!S->getType()->isRecordType()) { // The conditional operator can evaluate to either of the values of the // two branches. To model this, join these two values together to yield diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp index 66b3bba594fc9..14ac7ec4e39fe 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -17,6 +17,7 @@ #include "clang/Analysis/FlowSensitive/DataflowAnalysis.h" #include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h" #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" +#include "clang/Analysis/FlowSensitive/Formula.h" #include "clang/Analysis/FlowSensitive/NoopAnalysis.h" #include "clang/Analysis/FlowSensitive/NoopLattice.h" #include "clang/Analysis/FlowSensitive/RecordOps.h" @@ -4382,6 +4383,40 @@ TEST(TransferTest, VarDeclInitAssignConditionalOperator) { }); } +TEST(TransferTest, VarDeclInitReferenceAssignConditionalOperator) { + std::string Code = R"( + struct A { + int i; + }; + + void target(A Foo, A Bar, bool Cond) { + A &Baz = Cond ? Foo : Bar; + // Make sure A::i is modeled. + Baz.i; + /*[[p]]*/ + } + )"; + runDataflow( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); + + auto *FooIVal = cast<IntegerValue>(getFieldValue( + &getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "Foo"), "i", + ASTCtx, Env)); + auto *BarIVal = cast<IntegerValue>(getFieldValue( + &getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "Bar"), "i", + ASTCtx, Env)); + auto *BazIVal = cast<IntegerValue>(getFieldValue( + &getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "Baz"), "i", + ASTCtx, Env)); + + EXPECT_NE(BazIVal, FooIVal); + EXPECT_NE(BazIVal, BarIVal); + }); +} + TEST(TransferTest, VarDeclInDoWhile) { std::string Code = R"( void target(int *Foo) { @@ -6177,6 +6212,102 @@ TEST(TransferTest, ConditionalOperatorLocation) { }); } +TEST(TransferTest, ConditionalOperatorValuesTested) { + // Even though the LHS and the RHS of the conditional operator have different + // StorageLocations, we get constraints from the condition that the values + // must be equal to B1 for JoinDifferentIsB1, or B2 for JoinDifferentIsB2. + std::string Code = R"( + void target(bool B1, bool B2) { + bool JoinDifferentIsB1 = (B1 == B2) ? B2 : B1; + bool JoinDifferentIsB2 = (B1 == B2) ? B1 : B2; + // [[p]] + } + )"; + runDataflow( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + Environment Env = getEnvironmentAtAnnotation(Results, "p").fork(); + + auto &B1 = getValueForDecl<BoolValue>(ASTCtx, Env, "B1"); + auto &B2 = getValueForDecl<BoolValue>(ASTCtx, Env, "B2"); + auto &JoinDifferentIsB1 = + getValueForDecl<BoolValue>(ASTCtx, Env, "JoinDifferentIsB1"); + auto &JoinDifferentIsB2 = + getValueForDecl<BoolValue>(ASTCtx, Env, "JoinDifferentIsB2"); + + const Formula &JoinDifferentIsB1EqB1 = + Env.arena().makeEquals(JoinDifferentIsB1.formula(), B1.formula()); + EXPECT_TRUE(Env.allows(JoinDifferentIsB1EqB1)); + EXPECT_TRUE(Env.proves(JoinDifferentIsB1EqB1)); + + const Formula &JoinDifferentIsB2EqB2 = + Env.arena().makeEquals(JoinDifferentIsB2.formula(), B2.formula()); + EXPECT_TRUE(Env.allows(JoinDifferentIsB2EqB2)); + EXPECT_TRUE(Env.proves(JoinDifferentIsB2EqB2)); + }); +} + +TEST(TransferTest, ConditionalOperatorLocationUpdatedAfter) { + // We don't currently model a Conditional Operator with an LValue result + // as having aliases to the LHS and RHS (if it isn't just the same LValue + // on both sides). We also don't model that the update "may" happen + // (a weak update). So, we don't consider the LHS and RHS as being weakly + // updated at [[after_diff]]. + std::string Code = R"( + void target(bool Cond, bool B1, bool B2) { + (void)0; + // [[before_same]] + (Cond ? B1 : B1) = !B1; + // [[after_same]] + (Cond ? B1 : B2) = !B1; + // [[after_diff]] + } + )"; + runDataflow( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + Environment BeforeSameEnv = + getEnvironmentAtAnnotation(Results, "before_same").fork(); + Environment AfterSameEnv = + getEnvironmentAtAnnotation(Results, "after_same").fork(); + Environment AfterDiffEnv = + getEnvironmentAtAnnotation(Results, "after_diff").fork(); + + auto &BeforeSameB1 = + getValueForDecl<BoolValue>(ASTCtx, BeforeSameEnv, "B1"); + auto &AfterSameB1 = + getValueForDecl<BoolValue>(ASTCtx, AfterSameEnv, "B1"); + auto &AfterDiffB1 = + getValueForDecl<BoolValue>(ASTCtx, AfterDiffEnv, "B1"); + + EXPECT_NE(&BeforeSameB1, &AfterSameB1); + EXPECT_NE(&BeforeSameB1, &AfterDiffB1); + // FIXME: The formula for AfterSameB1 should be different from + // AfterDiffB1 to reflect that B1 may be updated. + EXPECT_EQ(&AfterSameB1, &AfterDiffB1); + + // The value of B1 is definitely different from before_same vs + // after_same. + const Formula &B1ChangedForSame = + AfterSameEnv.arena().makeNot(AfterSameEnv.arena().makeEquals( + AfterSameB1.formula(), BeforeSameB1.formula())); + EXPECT_TRUE(AfterSameEnv.allows(B1ChangedForSame)); + EXPECT_TRUE(AfterSameEnv.proves(B1ChangedForSame)); + + // FIXME: It should be possible that B1 *may* be updated, so it may be + // that AfterSameB1 != AfterDiffB1 or AfterSameB1 == AfterDiffB1. + const Formula &B1ChangedForDiff = + AfterDiffEnv.arena().makeNot(AfterDiffEnv.arena().makeEquals( + AfterDiffB1.formula(), AfterSameB1.formula())); + EXPECT_FALSE(AfterSameEnv.allows(B1ChangedForDiff)); + // proves() should be false, since B1 may or may not have changed + // depending on `Cond`. + EXPECT_FALSE(AfterSameEnv.proves(B1ChangedForDiff)); + }); +} + TEST(TransferTest, ConditionalOperatorOnConstantExpr) { // This is a regression test: We used to crash when a `ConstantExpr` was used // in the branches of a conditional operator. >From 5f376e09d44aac88e90d8af22c9302c282e5b423 Mon Sep 17 00:00:00 2001 From: Jan Voung <[email protected]> Date: Tue, 25 Nov 2025 02:09:19 +0000 Subject: [PATCH 2/2] Review comments --- clang/lib/Analysis/FlowSensitive/Transfer.cpp | 4 +- .../Analysis/FlowSensitive/TransferTest.cpp | 73 ++++++++++--------- 2 files changed, 40 insertions(+), 37 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp index 28c7fe3a5e1bc..05748359b7cef 100644 --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -783,12 +783,12 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> { // and joining the values right away, handling only case (a). // Otherwise, the dataflow framework needs to be updated be able to // represent alias sets and weak updates (for the "may"). - StorageLocation &Loc = Env.createStorageLocation(*S); - Env.setStorageLocation(*S, Loc); if (Value *Val = Environment::joinValues( S->getType(), TrueEnv->getValue(*S->getTrueExpr()), *TrueEnv, FalseEnv->getValue(*S->getFalseExpr()), *FalseEnv, Env, Model)) { + StorageLocation &Loc = Env.createStorageLocation(*S); + Env.setStorageLocation(*S, Loc); Env.setValue(Loc, *Val); } } diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp index 14ac7ec4e39fe..a6308d115aa70 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -6185,11 +6185,19 @@ TEST(TransferTest, ConditionalOperatorValue) { }); } -TEST(TransferTest, ConditionalOperatorLocation) { +TEST(TransferTest, ConditionalOperatorValuesTested) { + // We should be able to show that the result of the conditional operator, + // JoinResultMustBeB1, must be equal to B1, because the condition is checking + // `B1 == B2` and selecting B1 on the false branch, or B2 on the true branch. + // Similarly, for JoinResultMustBeB2 == B2. + // Note that the conditional operator involves a join of two *different* + // glvalues, before casting the lvalue to an rvalue, which may affect the + // implementation of the transfer function, and thus affect whether or not we + // can prove that IsB1 == B1. std::string Code = R"( - void target(bool Cond, int I1, int I2) { - int &JoinSame = Cond ? I1 : I1; - int &JoinDifferent = Cond ? I1 : I2; + void target(bool B1, bool B2) { + bool JoinResultMustBeB1 = (B1 == B2) ? B2 : B1; + bool JoinResultMustBeB2 = (B1 == B2) ? B1 : B2; // [[p]] } )"; @@ -6199,27 +6207,28 @@ TEST(TransferTest, ConditionalOperatorLocation) { ASTContext &ASTCtx) { Environment Env = getEnvironmentAtAnnotation(Results, "p").fork(); - StorageLocation &I1 = getLocForDecl(ASTCtx, Env, "I1"); - StorageLocation &I2 = getLocForDecl(ASTCtx, Env, "I2"); - StorageLocation &JoinSame = getLocForDecl(ASTCtx, Env, "JoinSame"); - StorageLocation &JoinDifferent = - getLocForDecl(ASTCtx, Env, "JoinDifferent"); + auto &B1 = getValueForDecl<BoolValue>(ASTCtx, Env, "B1"); + auto &B2 = getValueForDecl<BoolValue>(ASTCtx, Env, "B2"); + auto &JoinResultMustBeB1 = + getValueForDecl<BoolValue>(ASTCtx, Env, "JoinResultMustBeB1"); + auto &JoinResultMustBeB2 = + getValueForDecl<BoolValue>(ASTCtx, Env, "JoinResultMustBeB2"); - EXPECT_EQ(&JoinSame, &I1); + const Formula &MustBeB1_Eq_B1 = + Env.arena().makeEquals(JoinResultMustBeB1.formula(), B1.formula()); + EXPECT_TRUE(Env.proves(MustBeB1_Eq_B1)); - EXPECT_NE(&JoinDifferent, &I1); - EXPECT_NE(&JoinDifferent, &I2); + const Formula &MustBeB2_Eq_B2 = + Env.arena().makeEquals(JoinResultMustBeB2.formula(), B2.formula()); + EXPECT_TRUE(Env.proves(MustBeB2_Eq_B2)); }); } -TEST(TransferTest, ConditionalOperatorValuesTested) { - // Even though the LHS and the RHS of the conditional operator have different - // StorageLocations, we get constraints from the condition that the values - // must be equal to B1 for JoinDifferentIsB1, or B2 for JoinDifferentIsB2. +TEST(TransferTest, ConditionalOperatorLocation) { std::string Code = R"( - void target(bool B1, bool B2) { - bool JoinDifferentIsB1 = (B1 == B2) ? B2 : B1; - bool JoinDifferentIsB2 = (B1 == B2) ? B1 : B2; + void target(bool Cond, int I1, int I2) { + int &JoinSame = Cond ? I1 : I1; + int &JoinDifferent = Cond ? I1 : I2; // [[p]] } )"; @@ -6229,22 +6238,16 @@ TEST(TransferTest, ConditionalOperatorValuesTested) { ASTContext &ASTCtx) { Environment Env = getEnvironmentAtAnnotation(Results, "p").fork(); - auto &B1 = getValueForDecl<BoolValue>(ASTCtx, Env, "B1"); - auto &B2 = getValueForDecl<BoolValue>(ASTCtx, Env, "B2"); - auto &JoinDifferentIsB1 = - getValueForDecl<BoolValue>(ASTCtx, Env, "JoinDifferentIsB1"); - auto &JoinDifferentIsB2 = - getValueForDecl<BoolValue>(ASTCtx, Env, "JoinDifferentIsB2"); + StorageLocation &I1 = getLocForDecl(ASTCtx, Env, "I1"); + StorageLocation &I2 = getLocForDecl(ASTCtx, Env, "I2"); + StorageLocation &JoinSame = getLocForDecl(ASTCtx, Env, "JoinSame"); + StorageLocation &JoinDifferent = + getLocForDecl(ASTCtx, Env, "JoinDifferent"); - const Formula &JoinDifferentIsB1EqB1 = - Env.arena().makeEquals(JoinDifferentIsB1.formula(), B1.formula()); - EXPECT_TRUE(Env.allows(JoinDifferentIsB1EqB1)); - EXPECT_TRUE(Env.proves(JoinDifferentIsB1EqB1)); + EXPECT_EQ(&JoinSame, &I1); - const Formula &JoinDifferentIsB2EqB2 = - Env.arena().makeEquals(JoinDifferentIsB2.formula(), B2.formula()); - EXPECT_TRUE(Env.allows(JoinDifferentIsB2EqB2)); - EXPECT_TRUE(Env.proves(JoinDifferentIsB2EqB2)); + EXPECT_NE(&JoinDifferent, &I1); + EXPECT_NE(&JoinDifferent, &I2); }); } @@ -6296,11 +6299,11 @@ TEST(TransferTest, ConditionalOperatorLocationUpdatedAfter) { EXPECT_TRUE(AfterSameEnv.allows(B1ChangedForSame)); EXPECT_TRUE(AfterSameEnv.proves(B1ChangedForSame)); - // FIXME: It should be possible that B1 *may* be updated, so it may be - // that AfterSameB1 != AfterDiffB1 or AfterSameB1 == AfterDiffB1. const Formula &B1ChangedForDiff = AfterDiffEnv.arena().makeNot(AfterDiffEnv.arena().makeEquals( AfterDiffB1.formula(), AfterSameB1.formula())); + // FIXME: It should be possible that B1 *may* be updated, so it may be + // that AfterSameB1 != AfterDiffB1 or AfterSameB1 == AfterDiffB1. EXPECT_FALSE(AfterSameEnv.allows(B1ChangedForDiff)); // proves() should be false, since B1 may or may not have changed // depending on `Cond`. _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
