llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Jan Voung (jvoung) <details> <summary>Changes</summary> 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). --- Full diff: https://github.com/llvm/llvm-project/pull/168994.diff 2 Files Affected: - (modified) clang/lib/Analysis/FlowSensitive/Transfer.cpp (+22-1) - (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+131) ``````````diff 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. `````````` </details> https://github.com/llvm/llvm-project/pull/168994 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
