[clang] 62b2a47 - [clang][dataflow] Only skip ExprWithCleanups when visiting terminators
Eric Li via cfe-commits
cfe-commits at lists.llvm.org
Wed May 4 08:32:39 PDT 2022
Author: Eric Li
Date: 2022-05-04T15:31:49Z
New Revision: 62b2a47a9f15ed2f1dc4b39c924341c7b9bd7cf8
URL: https://github.com/llvm/llvm-project/commit/62b2a47a9f15ed2f1dc4b39c924341c7b9bd7cf8
DIFF: https://github.com/llvm/llvm-project/commit/62b2a47a9f15ed2f1dc4b39c924341c7b9bd7cf8.diff
LOG: [clang][dataflow] Only skip ExprWithCleanups when visiting terminators
`IgnoreParenImpCasts` will remove implicit casts to bool
(e.g. `PointerToBoolean`), such that the resulting expression may not
be of the `bool` type. The `cast_or_null<BoolValue>` in
`extendFlowCondition` will then trigger an assert, as the pointer
expression will not have a `BoolValue`.
Instead, we only skip `ExprWithCleanups` and `ParenExpr` nodes, as the
CFG does not emit them.
Differential Revision: https://reviews.llvm.org/D124807
Added:
Modified:
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
clang/include/clang/Analysis/FlowSensitive/Transfer.h
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/lib/Analysis/FlowSensitive/Transfer.cpp
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 0a2c75f804c2a..019d07c9b7f72 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -172,6 +172,10 @@ class Environment {
/// Creates a storage location for `E`. Does not assign the returned storage
/// location to `E` in the environment. Does not assign a value to the
/// returned storage location in the environment.
+ ///
+ /// Requirements:
+ ///
+ /// `E` must not be a `ExprWithCleanups`.
StorageLocation &createStorageLocation(const Expr &E);
/// Assigns `Loc` as the storage location of `D` in the environment.
@@ -191,11 +195,16 @@ class Environment {
/// Requirements:
///
/// `E` must not be assigned a storage location in the environment.
+ /// `E` must not be a `ExprWithCleanups`.
void setStorageLocation(const Expr &E, StorageLocation &Loc);
/// Returns the storage location assigned to `E` in the environment, applying
/// the `SP` policy for skipping past indirections, or null if `E` isn't
/// assigned a storage location in the environment.
+ ///
+ /// Requirements:
+ ///
+ /// `E` must not be a `ExprWithCleanups`.
StorageLocation *getStorageLocation(const Expr &E, SkipPast SP) const;
/// Returns the storage location assigned to the `this` pointee in the
@@ -226,6 +235,12 @@ class Environment {
/// Equivalent to `getValue(getStorageLocation(E, SP), SkipPast::None)` if `E`
/// is assigned a storage location in the environment, otherwise returns null.
+ ///
+ /// Requirements:
+ ///
+ /// `E` must not be a `ExprWithCleanups`.
+ ///
+ /// FIXME: `Environment` should ignore any `ExprWithCleanups` it sees.
Value *getValue(const Expr &E, SkipPast SP) const;
/// Transfers ownership of `Loc` to the analysis context and returns a
diff --git a/clang/include/clang/Analysis/FlowSensitive/Transfer.h b/clang/include/clang/Analysis/FlowSensitive/Transfer.h
index a6b663b997fd6..c5b1086c451fd 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Transfer.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Transfer.h
@@ -35,9 +35,19 @@ class StmtToEnvMap {
///
/// Requirements:
///
-/// The type of `S` must not be `ParenExpr`.
+/// `S` must not be `ParenExpr` or `ExprWithCleanups`.
void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env);
+/// Skip past a `ExprWithCleanups` which might surround `E`. Returns null if `E`
+/// is null.
+///
+/// The CFG omits `ExprWithCleanups` nodes (as it does with `ParenExpr`), and so
+/// the transfer function doesn't accept them as valid input. Manual traversal
+/// of the AST should skip and unwrap any `ExprWithCleanups` it might expect to
+/// see. They are safe to skip, as the CFG will emit calls to destructors as
+/// appropriate.
+const Expr *ignoreExprWithCleanups(const Expr *E);
+
} // namespace dataflow
} // namespace clang
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index 469696643d319..ad917f18b2570 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -15,6 +15,7 @@
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclCXX.h"
+#include "clang/AST/ExprCXX.h"
#include "clang/AST/Type.h"
#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
@@ -342,6 +343,7 @@ StorageLocation &Environment::createStorageLocation(const VarDecl &D) {
}
StorageLocation &Environment::createStorageLocation(const Expr &E) {
+ assert(!isa<ExprWithCleanups>(&E));
// Evaluated expressions are always assigned the same storage locations to
// ensure that the environment stabilizes across loop iterations. Storage
// locations for evaluated expressions are stored in the analysis context.
@@ -364,12 +366,14 @@ StorageLocation *Environment::getStorageLocation(const ValueDecl &D,
}
void Environment::setStorageLocation(const Expr &E, StorageLocation &Loc) {
+ assert(!isa<ExprWithCleanups>(&E));
assert(ExprToLoc.find(&E) == ExprToLoc.end());
ExprToLoc[&E] = &Loc;
}
StorageLocation *Environment::getStorageLocation(const Expr &E,
SkipPast SP) const {
+ assert(!isa<ExprWithCleanups>(&E));
// FIXME: Add a test with parens.
auto It = ExprToLoc.find(E.IgnoreParens());
return It == ExprToLoc.end() ? nullptr : &skip(*It->second, SP);
diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 52a8a4f0a8764..89d92787ea1cb 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -33,7 +33,7 @@
namespace clang {
namespace dataflow {
-static const Expr *skipExprWithCleanups(const Expr *E) {
+const Expr *ignoreExprWithCleanups(const Expr *E) {
if (auto *C = dyn_cast_or_null<ExprWithCleanups>(E))
return C->getSubExpr();
return E;
@@ -155,9 +155,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
return;
}
- // The CFG does not contain `ParenExpr` as top-level statements in basic
- // blocks, however sub-expressions can still be of that type.
- InitExpr = skipExprWithCleanups(D.getInit()->IgnoreParens());
+ InitExpr = ignoreExprWithCleanups(D.getInit());
assert(InitExpr != nullptr);
if (D.getType()->isReferenceType()) {
@@ -190,10 +188,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
}
void VisitImplicitCastExpr(const ImplicitCastExpr *S) {
- // The CFG does not contain `ParenExpr` as top-level statements in basic
- // blocks, however sub-expressions can still be of that type.
- assert(S->getSubExpr() != nullptr);
- const Expr *SubExpr = S->getSubExpr()->IgnoreParens();
+ const Expr *SubExpr = S->getSubExpr();
assert(SubExpr != nullptr);
switch (S->getCastKind()) {
@@ -252,10 +247,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
}
void VisitUnaryOperator(const UnaryOperator *S) {
- // The CFG does not contain `ParenExpr` as top-level statements in basic
- // blocks, however sub-expressions can still be of that type.
- assert(S->getSubExpr() != nullptr);
- const Expr *SubExpr = S->getSubExpr()->IgnoreParens();
+ const Expr *SubExpr = S->getSubExpr();
assert(SubExpr != nullptr);
switch (S->getOpcode()) {
@@ -444,9 +436,6 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
void VisitCXXFunctionalCastExpr(const CXXFunctionalCastExpr *S) {
if (S->getCastKind() == CK_ConstructorConversion) {
- // The CFG does not contain `ParenExpr` as top-level statements in basic
- // blocks, however sub-expressions can still be of that type.
- assert(S->getSubExpr() != nullptr);
const Expr *SubExpr = S->getSubExpr();
assert(SubExpr != nullptr);
@@ -604,7 +593,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
};
void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env) {
- assert(!isa<ParenExpr>(&S));
+ assert(!(isa<ParenExpr, ExprWithCleanups>(&S)));
TransferVisitor(StmtToEnv, Env).Visit(&S);
}
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 72e6405724608..cc94b8bed2f7f 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -77,26 +77,26 @@ class TerminatorVisitor : public ConstStmtVisitor<TerminatorVisitor> {
: StmtToEnv(StmtToEnv), Env(Env), BlockSuccIdx(BlockSuccIdx) {}
void VisitIfStmt(const IfStmt *S) {
- auto *Cond = S->getCond()->IgnoreParenImpCasts();
+ auto *Cond = ignoreExprWithCleanups(S->getCond())->IgnoreParens();
assert(Cond != nullptr);
extendFlowCondition(*Cond);
}
void VisitWhileStmt(const WhileStmt *S) {
- auto *Cond = S->getCond()->IgnoreParenImpCasts();
+ auto *Cond = ignoreExprWithCleanups(S->getCond())->IgnoreParens();
assert(Cond != nullptr);
extendFlowCondition(*Cond);
}
void VisitBinaryOperator(const BinaryOperator *S) {
assert(S->getOpcode() == BO_LAnd || S->getOpcode() == BO_LOr);
- auto *LHS = S->getLHS()->IgnoreParenImpCasts();
+ auto *LHS = ignoreExprWithCleanups(S->getLHS())->IgnoreParens();
assert(LHS != nullptr);
extendFlowCondition(*LHS);
}
void VisitConditionalOperator(const ConditionalOperator *S) {
- auto *Cond = S->getCond()->IgnoreParenImpCasts();
+ auto *Cond = ignoreExprWithCleanups(S->getCond())->IgnoreParens();
assert(Cond != nullptr);
extendFlowCondition(*Cond);
}
diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index e7abd497b5b62..2509aa3b9d8dd 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -1152,4 +1152,38 @@ TEST_F(FlowConditionTest, OpaqueFlowConditionInsideBranchMergesToOpaqueBool) {
});
}
+TEST_F(FlowConditionTest, PointerToBoolImplicitCast) {
+ std::string Code = R"(
+ void target(int *Ptr) {
+ bool Foo = false;
+ if (Ptr) {
+ Foo = true;
+ /*[[p1]]*/
+ }
+
+ (void)0;
+ /*[[p2]]*/
+ }
+ )";
+ runDataflow(
+ Code, [](llvm::ArrayRef<
+ std::pair<std::string, DataflowAnalysisState<NoopLattice>>>
+ Results,
+ ASTContext &ASTCtx) {
+ ASSERT_THAT(Results, ElementsAre(Pair("p2", _), Pair("p1", _)));
+ const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
+ ASSERT_THAT(FooDecl, NotNull());
+
+ const Environment &Env1 = Results[1].second.Env;
+ auto &FooVal1 =
+ *cast<BoolValue>(Env1.getValue(*FooDecl, SkipPast::Reference));
+ EXPECT_TRUE(Env1.flowConditionImplies(FooVal1));
+
+ const Environment &Env2 = Results[0].second.Env;
+ auto &FooVal2 =
+ *cast<BoolValue>(Env2.getValue(*FooDecl, SkipPast::Reference));
+ EXPECT_FALSE(Env2.flowConditionImplies(FooVal2));
+ });
+}
+
} // namespace
More information about the cfe-commits
mailing list