ymandel updated this revision to Diff 423690.
ymandel added a comment.
add comments to tests.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123858/new/
https://reviews.llvm.org/D123858
Files:
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
Index: clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
===================================================================
--- clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -878,4 +878,119 @@
});
}
+// Verifies that flow conditions are properly constructed even when the
+// condition is not meaningfully interpreted.
+//
+// Note: currently, abstract 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
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ 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,23 @@
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 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) {
+ 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.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
