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 { 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> &Results, + ASTContext &ASTCtx) { + const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); + + auto *FooIVal = cast(getFieldValue( + &getLocForDecl(ASTCtx, Env, "Foo"), "i", + ASTCtx, Env)); + auto *BarIVal = cast(getFieldValue( + &getLocForDecl(ASTCtx, Env, "Bar"), "i", + ASTCtx, Env)); + auto *BazIVal = cast(getFieldValue( + &getLocForDecl(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> &Results, + ASTContext &ASTCtx) { + Environment Env = getEnvironmentAtAnnotation(Results, "p").fork(); + + auto &B1 = getValueForDecl(ASTCtx, Env, "B1"); + auto &B2 = getValueForDecl(ASTCtx, Env, "B2"); + auto &JoinDifferentIsB1 = + getValueForDecl(ASTCtx, Env, "JoinDifferentIsB1"); + auto &JoinDifferentIsB2 = + getValueForDecl(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> &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(ASTCtx, BeforeSameEnv, "B1"); + auto &AfterSameB1 = + getValueForDecl(ASTCtx, AfterSameEnv, "B1"); + auto &AfterDiffB1 = + getValueForDecl(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.