[clang] 37b4782 - [clang][dataflow] Fix `Environment::join`'s handling of flow-condition merging.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 25 08:07:07 PDT 2022
Author: Yitzhak Mandelbaum
Date: 2022-04-25T15:05:50Z
New Revision: 37b4782e3e53cba265d26843f222134ed21e1974
URL: https://github.com/llvm/llvm-project/commit/37b4782e3e53cba265d26843f222134ed21e1974
DIFF: https://github.com/llvm/llvm-project/commit/37b4782e3e53cba265d26843f222134ed21e1974.diff
LOG: [clang][dataflow] Fix `Environment::join`'s handling of flow-condition merging.
The current implementation mutates the environment as it performs the
join. However, that interferes with the call to the model's `merge` operation,
which can modify `MergedEnv`. Since any modifications are assumed to apply to
the post-join environment, providing the same environment for both is
incorrect. This mismatch is a particular concern for joining the flow
conditions, where modifications in the old environment may not be propagated to
the new one.
Differential Revision: https://reviews.llvm.org/D124104
Added:
Modified:
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
Removed:
################################################################################
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 04bf41605d9d3..91e07bdb88ce0 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -66,12 +66,14 @@ static bool equivalentValues(QualType Type, Value *Val1,
return Model.compareEquivalent(Type, *Val1, Env1, *Val2, Env2);
}
-/// Attempts to merge distinct values `Val1` and `Val1` in `Env1` and `Env2`,
+/// Attempts to merge distinct values `Val1` and `Val2` in `Env1` and `Env2`,
/// respectively, of the same type `Type`. Merging generally produces a single
/// value that (soundly) approximates the two inputs, although the actual
/// meaning depends on `Model`.
-static Value *mergeDistinctValues(QualType Type, Value *Val1, Environment &Env1,
- Value *Val2, const Environment &Env2,
+static Value *mergeDistinctValues(QualType Type, Value *Val1,
+ const Environment &Env1, Value *Val2,
+ const Environment &Env2,
+ Environment &MergedEnv,
Environment::ValueModel &Model) {
// Join distinct boolean values preserving information about the constraints
// in the respective path conditions. Note: this construction can, in
@@ -94,19 +96,19 @@ static Value *mergeDistinctValues(QualType Type, Value *Val1, Environment &Env1,
// have mutually exclusive conditions.
if (auto *Expr1 = dyn_cast<BoolValue>(Val1)) {
for (BoolValue *Constraint : Env1.getFlowConditionConstraints()) {
- Expr1 = &Env1.makeAnd(*Expr1, *Constraint);
+ Expr1 = &MergedEnv.makeAnd(*Expr1, *Constraint);
}
auto *Expr2 = cast<BoolValue>(Val2);
for (BoolValue *Constraint : Env2.getFlowConditionConstraints()) {
- Expr2 = &Env1.makeAnd(*Expr2, *Constraint);
+ Expr2 = &MergedEnv.makeAnd(*Expr2, *Constraint);
}
- return &Env1.makeOr(*Expr1, *Expr2);
+ return &MergedEnv.makeOr(*Expr1, *Expr2);
}
// FIXME: Consider destroying `MergedValue` immediately if `ValueModel::merge`
// returns false to avoid storing unneeded values in `DACtx`.
- if (Value *MergedVal = Env1.createValue(Type))
- if (Model.merge(Type, *Val1, Env1, *Val2, Env2, *MergedVal, Env1))
+ if (Value *MergedVal = MergedEnv.createValue(Type))
+ if (Model.merge(Type, *Val1, Env1, *Val2, Env2, *MergedVal, MergedEnv))
return MergedVal;
return nullptr;
@@ -315,28 +317,26 @@ LatticeJoinEffect Environment::join(const Environment &Other,
auto Effect = LatticeJoinEffect::Unchanged;
- const unsigned DeclToLocSizeBefore = DeclToLoc.size();
- DeclToLoc = intersectDenseMaps(DeclToLoc, Other.DeclToLoc);
- if (DeclToLocSizeBefore != DeclToLoc.size())
+ Environment JoinedEnv(*DACtx);
+
+ JoinedEnv.DeclToLoc = intersectDenseMaps(DeclToLoc, Other.DeclToLoc);
+ if (DeclToLoc.size() != JoinedEnv.DeclToLoc.size())
Effect = LatticeJoinEffect::Changed;
- const unsigned ExprToLocSizeBefore = ExprToLoc.size();
- ExprToLoc = intersectDenseMaps(ExprToLoc, Other.ExprToLoc);
- if (ExprToLocSizeBefore != ExprToLoc.size())
+ JoinedEnv.ExprToLoc = intersectDenseMaps(ExprToLoc, Other.ExprToLoc);
+ if (ExprToLoc.size() != JoinedEnv.ExprToLoc.size())
Effect = LatticeJoinEffect::Changed;
- const unsigned MemberLocToStructSizeBefore = MemberLocToStruct.size();
- MemberLocToStruct =
+ JoinedEnv.MemberLocToStruct =
intersectDenseMaps(MemberLocToStruct, Other.MemberLocToStruct);
- if (MemberLocToStructSizeBefore != MemberLocToStruct.size())
+ if (MemberLocToStruct.size() != JoinedEnv.MemberLocToStruct.size())
Effect = LatticeJoinEffect::Changed;
- // Move `LocToVal` so that `Environment::ValueModel::merge` can safely assign
- // values to storage locations while this code iterates over the current
- // assignments.
- llvm::DenseMap<const StorageLocation *, Value *> OldLocToVal =
- std::move(LocToVal);
- for (auto &Entry : OldLocToVal) {
+ // FIXME: set `Effect` as needed.
+ JoinedEnv.FlowConditionConstraints = joinConstraints(
+ DACtx, FlowConditionConstraints, Other.FlowConditionConstraints);
+
+ for (auto &Entry : LocToVal) {
const StorageLocation *Loc = Entry.first;
assert(Loc != nullptr);
@@ -349,19 +349,18 @@ LatticeJoinEffect Environment::join(const Environment &Other,
assert(It->second != nullptr);
if (Val == It->second) {
- LocToVal.insert({Loc, Val});
+ JoinedEnv.LocToVal.insert({Loc, Val});
continue;
}
- if (Value *MergedVal = mergeDistinctValues(Loc->getType(), Val, *this,
- It->second, Other, Model))
- LocToVal.insert({Loc, MergedVal});
+ if (Value *MergedVal = mergeDistinctValues(
+ Loc->getType(), Val, *this, It->second, Other, JoinedEnv, Model))
+ JoinedEnv.LocToVal.insert({Loc, MergedVal});
}
- if (OldLocToVal.size() != LocToVal.size())
+ if (LocToVal.size() != JoinedEnv.LocToVal.size())
Effect = LatticeJoinEffect::Changed;
- FlowConditionConstraints = joinConstraints(DACtx, FlowConditionConstraints,
- Other.FlowConditionConstraints);
+ *this = std::move(JoinedEnv);
return Effect;
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index 0a469dbd73dd4..e7abd497b5b62 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -10,6 +10,7 @@
#include "TestingSupport.h"
#include "clang/AST/Decl.h"
#include "clang/AST/ExprCXX.h"
+#include "clang/AST/Type.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Analysis/CFG.h"
@@ -295,6 +296,164 @@ TEST_F(NoreturnDestructorTest, ConditionalOperatorNestedBranchReturns) {
// FIXME: Called functions at point `p` should contain only "foo".
}
+// Models an analysis that uses flow conditions.
+class SpecialBoolAnalysis
+ : public DataflowAnalysis<SpecialBoolAnalysis, NoopLattice> {
+public:
+ explicit SpecialBoolAnalysis(ASTContext &Context)
+ : DataflowAnalysis<SpecialBoolAnalysis, NoopLattice>(Context) {}
+
+ static NoopLattice initialElement() { return {}; }
+
+ void transfer(const Stmt *S, NoopLattice &, Environment &Env) {
+ auto SpecialBoolRecordDecl = recordDecl(hasName("SpecialBool"));
+ auto HasSpecialBoolType = hasType(SpecialBoolRecordDecl);
+
+ if (const auto *E = selectFirst<CXXConstructExpr>(
+ "call", match(cxxConstructExpr(HasSpecialBoolType).bind("call"), *S,
+ getASTContext()))) {
+ auto &ConstructorVal = *cast<StructValue>(Env.createValue(E->getType()));
+ ConstructorVal.setProperty("is_set", Env.getBoolLiteralValue(false));
+ Env.setValue(*Env.getStorageLocation(*E, SkipPast::None), ConstructorVal);
+ } else if (const auto *E = selectFirst<CXXMemberCallExpr>(
+ "call", match(cxxMemberCallExpr(callee(cxxMethodDecl(ofClass(
+ SpecialBoolRecordDecl))))
+ .bind("call"),
+ *S, getASTContext()))) {
+ auto *Object = E->getImplicitObjectArgument();
+ assert(Object != nullptr);
+
+ auto *ObjectLoc =
+ Env.getStorageLocation(*Object, SkipPast::ReferenceThenPointer);
+ assert(ObjectLoc != nullptr);
+
+ auto &ConstructorVal =
+ *cast<StructValue>(Env.createValue(Object->getType()));
+ ConstructorVal.setProperty("is_set", Env.getBoolLiteralValue(true));
+ Env.setValue(*ObjectLoc, ConstructorVal);
+ }
+ }
+
+ bool compareEquivalent(QualType Type, const Value &Val1,
+ const Environment &Env1, const Value &Val2,
+ const Environment &Env2) final {
+ const auto *Decl = Type->getAsCXXRecordDecl();
+ if (Decl == nullptr || Decl->getIdentifier() == nullptr ||
+ Decl->getName() != "SpecialBool")
+ return false;
+
+ auto *IsSet1 = cast_or_null<BoolValue>(
+ cast<StructValue>(&Val1)->getProperty("is_set"));
+ if (IsSet1 == nullptr)
+ return true;
+
+ auto *IsSet2 = cast_or_null<BoolValue>(
+ cast<StructValue>(&Val2)->getProperty("is_set"));
+ if (IsSet2 == nullptr)
+ return false;
+
+ return Env1.flowConditionImplies(*IsSet1) ==
+ Env2.flowConditionImplies(*IsSet2);
+ }
+
+ // Always returns `true` to accept the `MergedVal`.
+ bool merge(QualType Type, const Value &Val1, const Environment &Env1,
+ const Value &Val2, const Environment &Env2, Value &MergedVal,
+ Environment &MergedEnv) final {
+ const auto *Decl = Type->getAsCXXRecordDecl();
+ if (Decl == nullptr || Decl->getIdentifier() == nullptr ||
+ Decl->getName() != "SpecialBool")
+ return true;
+
+ auto *IsSet1 = cast_or_null<BoolValue>(
+ cast<StructValue>(&Val1)->getProperty("is_set"));
+ if (IsSet1 == nullptr)
+ return true;
+
+ auto *IsSet2 = cast_or_null<BoolValue>(
+ cast<StructValue>(&Val2)->getProperty("is_set"));
+ if (IsSet2 == nullptr)
+ return true;
+
+ auto &IsSet = MergedEnv.makeAtomicBoolValue();
+ cast<StructValue>(&MergedVal)->setProperty("is_set", IsSet);
+ if (Env1.flowConditionImplies(*IsSet1) &&
+ Env2.flowConditionImplies(*IsSet2))
+ MergedEnv.addToFlowCondition(IsSet);
+
+ return true;
+ }
+};
+
+class JoinFlowConditionsTest : public Test {
+protected:
+ template <typename Matcher>
+ void runDataflow(llvm::StringRef Code, Matcher Match) {
+ ASSERT_THAT_ERROR(
+ test::checkDataflow<SpecialBoolAnalysis>(
+ Code, "target",
+ [](ASTContext &Context, Environment &Env) {
+ return SpecialBoolAnalysis(Context);
+ },
+ [&Match](
+ llvm::ArrayRef<
+ std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
+ Results,
+ ASTContext &ASTCtx) { Match(Results, ASTCtx); },
+ {"-fsyntax-only", "-std=c++17"}),
+ llvm::Succeeded());
+ }
+};
+
+TEST_F(JoinFlowConditionsTest, JoinDistinctButProvablyEquivalentValues) {
+ std::string Code = R"(
+ struct SpecialBool {
+ SpecialBool() = default;
+ void set();
+ };
+
+ void target(bool Cond) {
+ SpecialBool Foo;
+ /*[[p1]]*/
+ if (Cond) {
+ Foo.set();
+ /*[[p2]]*/
+ } else {
+ Foo.set();
+ /*[[p3]]*/
+ }
+ (void)0;
+ /*[[p4]]*/
+ }
+ )";
+ runDataflow(Code,
+ [](llvm::ArrayRef<
+ std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
+ Results,
+ ASTContext &ASTCtx) {
+ ASSERT_THAT(Results, ElementsAre(Pair("p4", _), Pair("p3", _),
+ Pair("p2", _), Pair("p1", _)));
+ const Environment &Env1 = Results[3].second.Env;
+ const Environment &Env2 = Results[2].second.Env;
+ const Environment &Env3 = Results[1].second.Env;
+ const Environment &Env4 = Results[0].second.Env;
+
+ const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ ASSERT_THAT(FooDecl, NotNull());
+
+ auto GetFooValue = [FooDecl](const Environment &Env) {
+ return cast<BoolValue>(
+ cast<StructValue>(Env.getValue(*FooDecl, SkipPast::None))
+ ->getProperty("is_set"));
+ };
+
+ EXPECT_FALSE(Env1.flowConditionImplies(*GetFooValue(Env1)));
+ EXPECT_TRUE(Env2.flowConditionImplies(*GetFooValue(Env2)));
+ EXPECT_TRUE(Env3.flowConditionImplies(*GetFooValue(Env3)));
+ EXPECT_TRUE(Env4.flowConditionImplies(*GetFooValue(Env3)));
+ });
+}
+
class OptionalIntAnalysis
: public DataflowAnalysis<OptionalIntAnalysis, NoopLattice> {
public:
More information about the cfe-commits
mailing list