Author: Yitzhak Mandelbaum Date: 2022-04-20T17:01:55Z New Revision: c8f822ad51951094504866049546bd2c3446728f
URL: https://github.com/llvm/llvm-project/commit/c8f822ad51951094504866049546bd2c3446728f DIFF: https://github.com/llvm/llvm-project/commit/c8f822ad51951094504866049546bd2c3446728f.diff LOG: [clang][dataflow] Ensure well-formed flow conditions. Ensure that the expressions associated with terminators are associated with a value. Otherwise, we can generate degenerate flow conditions, where both branches share the same condition. Differential Revision: https://reviews.llvm.org/D123858 Added: Modified: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp Removed: ################################################################################ diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp index 6baf1a7fd42d6..72e6405724608 100644 --- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp +++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp @@ -32,6 +32,7 @@ #include "llvm/ADT/Optional.h" #include "llvm/ADT/STLExtras.h" #include "llvm/Support/Error.h" +#include "llvm/Support/ErrorHandling.h" namespace clang { namespace dataflow { @@ -106,10 +107,24 @@ class TerminatorVisitor : public ConstStmtVisitor<TerminatorVisitor> { if (Env.getValue(Cond, SkipPast::None) == nullptr) transfer(StmtToEnv, Cond, Env); + // FIXME: The flow condition must be an r-value, so `SkipPast::None` should + // suffice. auto *Val = cast_or_null<BoolValue>(Env.getValue(Cond, SkipPast::Reference)); - if (Val == nullptr) - return; + // Value merging depends on flow conditions from diff erent environments + // being mutually exclusive -- that is, they cannot both be true in their + // entirety (even if they may share some clauses). So, we need *some* value + // for the condition expression, even if just an atom. + if (Val == nullptr) { + // FIXME: Consider introducing a helper for this get-or-create pattern. + auto *Loc = Env.getStorageLocation(Cond, SkipPast::None); + if (Loc == nullptr) { + Loc = &Env.createStorageLocation(Cond); + Env.setStorageLocation(Cond, *Loc); + } + Val = &Env.makeAtomicBoolValue(); + Env.setValue(*Loc, *Val); + } // The condition must be inverted for the successor that encompasses the // "else" branch, if such exists. diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp index 2f5185a47e3e2..0a469dbd73dd4 100644 --- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp @@ -878,4 +878,119 @@ TEST_F(FlowConditionTest, Join) { }); } +// Verifies that flow conditions are properly constructed even when the +// condition is not meaningfully interpreted. +// +// Note: currently, arbitrary function calls are uninterpreted, so the test +// exercises this case. If and when we change that, this test will not add to +// coverage (although it may still test a valuable case). +TEST_F(FlowConditionTest, OpaqueFlowConditionMergesToOpaqueBool) { + std::string Code = R"( + bool foo(); + + void target() { + bool Bar = true; + if (foo()) + Bar = false; + (void)0; + /*[[p]]*/ + } + )"; + runDataflow( + Code, [](llvm::ArrayRef< + std::pair<std::string, DataflowAnalysisState<NoopLattice>>> + Results, + ASTContext &ASTCtx) { + ASSERT_THAT(Results, ElementsAre(Pair("p", _))); + const Environment &Env = Results[0].second.Env; + + const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar"); + ASSERT_THAT(BarDecl, NotNull()); + + auto &BarVal = + *cast<BoolValue>(Env.getValue(*BarDecl, SkipPast::Reference)); + + EXPECT_FALSE(Env.flowConditionImplies(BarVal)); + }); +} + +// Verifies that flow conditions are properly constructed even when the +// condition is not meaningfully interpreted. +// +// Note: currently, fields with recursive type calls are uninterpreted (beneath +// the first instance), so the test exercises this case. If and when we change +// that, this test will not add to coverage (although it may still test a +// valuable case). +TEST_F(FlowConditionTest, OpaqueFieldFlowConditionMergesToOpaqueBool) { + std::string Code = R"( + struct Rec { + Rec* Next; + }; + + struct Foo { + Rec* X; + }; + + void target(Foo F) { + bool Bar = true; + if (F.X->Next) + Bar = false; + (void)0; + /*[[p]]*/ + } + )"; + runDataflow( + Code, [](llvm::ArrayRef< + std::pair<std::string, DataflowAnalysisState<NoopLattice>>> + Results, + ASTContext &ASTCtx) { + ASSERT_THAT(Results, ElementsAre(Pair("p", _))); + const Environment &Env = Results[0].second.Env; + + const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar"); + ASSERT_THAT(BarDecl, NotNull()); + + auto &BarVal = + *cast<BoolValue>(Env.getValue(*BarDecl, SkipPast::Reference)); + + EXPECT_FALSE(Env.flowConditionImplies(BarVal)); + }); +} + +// Verifies that flow conditions are properly constructed even when the +// condition is not meaningfully interpreted. Adds to above by nesting the +// interestnig case inside a normal branch. This protects against degenerate +// solutions which only test for empty flow conditions, for example. +TEST_F(FlowConditionTest, OpaqueFlowConditionInsideBranchMergesToOpaqueBool) { + std::string Code = R"( + bool foo(); + + void target(bool Cond) { + bool Bar = true; + if (Cond) { + if (foo()) + Bar = false; + (void)0; + /*[[p]]*/ + } + } + )"; + runDataflow( + Code, [](llvm::ArrayRef< + std::pair<std::string, DataflowAnalysisState<NoopLattice>>> + Results, + ASTContext &ASTCtx) { + ASSERT_THAT(Results, ElementsAre(Pair("p", _))); + const Environment &Env = Results[0].second.Env; + + const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar"); + ASSERT_THAT(BarDecl, NotNull()); + + auto &BarVal = + *cast<BoolValue>(Env.getValue(*BarDecl, SkipPast::Reference)); + + EXPECT_FALSE(Env.flowConditionImplies(BarVal)); + }); +} + } // namespace _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
