From c8f822ad51951094504866049546bd2c3446728f Mon Sep 17 00:00:00 2001 From: Yitzhak Mandelbaum Date: Mon, 11 Apr 2022 19:41:42 +0000 Subject: [PATCH] [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 --- .../TypeErasedDataflowAnalysis.cpp | 19 ++- .../TypeErasedDataflowAnalysisTest.cpp | 115 ++++++++++++++++++ 2 files changed, 132 insertions(+), 2 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp index 6baf1a7fd42d..72e640572460 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 @@ private: 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(Env.getValue(Cond, SkipPast::Reference)); - if (Val == nullptr) - return; + // Value merging depends on flow conditions from different 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 2f5185a47e3e..0a469dbd73dd 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>> + 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(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>> + 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(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>> + 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(Env.getValue(*BarDecl, SkipPast::Reference)); + + EXPECT_FALSE(Env.flowConditionImplies(BarVal)); + }); +} + } // namespace