[clang] 45643cf - [clang][dataflow] Centralize expression skipping logic
Eric Li via cfe-commits
cfe-commits at lists.llvm.org
Thu May 5 13:41:49 PDT 2022
Author: Eric Li
Date: 2022-05-05T20:28:11Z
New Revision: 45643cfcc12ea6152fb9516ed24f5adf7469eb4c
URL: https://github.com/llvm/llvm-project/commit/45643cfcc12ea6152fb9516ed24f5adf7469eb4c
DIFF: https://github.com/llvm/llvm-project/commit/45643cfcc12ea6152fb9516ed24f5adf7469eb4c.diff
LOG: [clang][dataflow] Centralize expression skipping logic
A follow-up to 62b2a47 to centralize the logic that skips expressions
that the CFG does not emit. This allows client code to avoid
sprinkling this logic everywhere.
Add redirects in the transfer function to similarly skip such
expressions by forwarding the visit to the sub-expression.
Differential Revision: https://reviews.llvm.org/D124965
Added:
Modified:
clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
clang/include/clang/Analysis/FlowSensitive/Transfer.h
clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
clang/lib/Analysis/FlowSensitive/Transfer.cpp
clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
index a762cb9de48bd..e2820fcb55655 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
@@ -31,6 +31,19 @@
namespace clang {
namespace dataflow {
+/// Skip past nodes that the CFG does not emit. These nodes are invisible to
+/// flow-sensitive analysis, and should be ignored as they will effectively not
+/// exist.
+///
+/// * `ParenExpr` - The CFG takes the operator precedence into account, but
+/// otherwise omits the node afterwards.
+///
+/// * `ExprWithCleanups` - The CFG will generate the appropriate calls to
+/// destructors and then omit the node.
+///
+const Expr &ignoreCFGOmittedNodes(const Expr &E);
+const Stmt &ignoreCFGOmittedNodes(const Stmt &S);
+
/// Owns objects that encompass the state of a program and stores context that
/// is used during dataflow analysis.
class DataflowAnalysisContext {
@@ -95,14 +108,15 @@ class DataflowAnalysisContext {
///
/// `E` must not be assigned a storage location.
void setStorageLocation(const Expr &E, StorageLocation &Loc) {
- assert(ExprToLoc.find(&E) == ExprToLoc.end());
- ExprToLoc[&E] = &Loc;
+ const Expr &CanonE = ignoreCFGOmittedNodes(E);
+ assert(ExprToLoc.find(&CanonE) == ExprToLoc.end());
+ ExprToLoc[&CanonE] = &Loc;
}
/// Returns the storage location assigned to `E` or null if `E` has no
/// assigned storage location.
StorageLocation *getStorageLocation(const Expr &E) const {
- auto It = ExprToLoc.find(&E);
+ auto It = ExprToLoc.find(&ignoreCFGOmittedNodes(E));
return It == ExprToLoc.end() ? nullptr : It->second;
}
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
index 019d07c9b7f72..0a2c75f804c2a 100644
--- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -172,10 +172,6 @@ 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.
@@ -195,16 +191,11 @@ 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
@@ -235,12 +226,6 @@ 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 c5b1086c451fd..25afa01f307c0 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Transfer.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Transfer.h
@@ -38,16 +38,6 @@ class StmtToEnvMap {
/// `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/DataflowAnalysisContext.cpp b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
index b29983eed79f9..81e37e6e6905a 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
@@ -13,6 +13,7 @@
//===----------------------------------------------------------------------===//
#include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
+#include "clang/AST/ExprCXX.h"
#include "clang/Analysis/FlowSensitive/Value.h"
#include <cassert>
#include <memory>
@@ -155,3 +156,22 @@ void DataflowAnalysisContext::addTransitiveFlowConditionConstraints(
} // namespace dataflow
} // namespace clang
+
+using namespace clang;
+
+const Expr &clang::dataflow::ignoreCFGOmittedNodes(const Expr &E) {
+ const Expr *Current = &E;
+ if (auto *EWC = dyn_cast<ExprWithCleanups>(Current)) {
+ Current = EWC->getSubExpr();
+ assert(Current != nullptr);
+ }
+ Current = Current->IgnoreParens();
+ assert(Current != nullptr);
+ return *Current;
+}
+
+const Stmt &clang::dataflow::ignoreCFGOmittedNodes(const Stmt &S) {
+ if (auto *E = dyn_cast<Expr>(&S))
+ return ignoreCFGOmittedNodes(*E);
+ return S;
+}
diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
index ad917f18b2570..51aea12cb2714 100644
--- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -343,7 +343,6 @@ 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.
@@ -366,16 +365,15 @@ 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;
+ const Expr &CanonE = ignoreCFGOmittedNodes(E);
+ assert(ExprToLoc.find(&CanonE) == ExprToLoc.end());
+ ExprToLoc[&CanonE] = &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());
+ auto It = ExprToLoc.find(&ignoreCFGOmittedNodes(E));
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 89d92787ea1cb..3a94434b7aeb7 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -33,27 +33,12 @@
namespace clang {
namespace dataflow {
-const Expr *ignoreExprWithCleanups(const Expr *E) {
- if (auto *C = dyn_cast_or_null<ExprWithCleanups>(E))
- return C->getSubExpr();
- return E;
-}
-
static BoolValue &evaluateBooleanEquality(const Expr &LHS, const Expr &RHS,
Environment &Env) {
- // Equality of booleans involves implicit integral casts. Ignore these casts
- // for now and focus on the values associated with the wrapped expressions.
- // FIXME: Consider changing this once the framework offers better support for
- // integral casts.
- const Expr *LHSNorm = LHS.IgnoreCasts();
- const Expr *RHSNorm = RHS.IgnoreCasts();
- assert(LHSNorm != nullptr);
- assert(RHSNorm != nullptr);
-
- if (auto *LHSValue = dyn_cast_or_null<BoolValue>(
- Env.getValue(*LHSNorm, SkipPast::Reference)))
- if (auto *RHSValue = dyn_cast_or_null<BoolValue>(
- Env.getValue(*RHSNorm, SkipPast::Reference)))
+ if (auto *LHSValue =
+ dyn_cast_or_null<BoolValue>(Env.getValue(LHS, SkipPast::Reference)))
+ if (auto *RHSValue =
+ dyn_cast_or_null<BoolValue>(Env.getValue(RHS, SkipPast::Reference)))
return Env.makeIff(*LHSValue, *RHSValue);
return Env.makeAtomicBoolValue();
@@ -65,14 +50,10 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
: StmtToEnv(StmtToEnv), Env(Env) {}
void VisitBinaryOperator(const BinaryOperator *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->getLHS() != nullptr);
- const Expr *LHS = S->getLHS()->IgnoreParens();
+ const Expr *LHS = S->getLHS();
assert(LHS != nullptr);
- assert(S->getRHS() != nullptr);
- const Expr *RHS = S->getRHS()->IgnoreParens();
+ const Expr *RHS = S->getRHS();
assert(RHS != nullptr);
switch (S->getOpcode()) {
@@ -155,7 +136,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
return;
}
- InitExpr = ignoreExprWithCleanups(D.getInit());
+ InitExpr = D.getInit();
assert(InitExpr != nullptr);
if (D.getType()->isReferenceType()) {
@@ -476,8 +457,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
assert(S->getArg(0) != nullptr);
// `__builtin_expect` returns by-value, so strip away any potential
// references in the argument.
- auto *ArgLoc = Env.getStorageLocation(
- *S->getArg(0)->IgnoreParenImpCasts(), SkipPast::Reference);
+ auto *ArgLoc = Env.getStorageLocation(*S->getArg(0), SkipPast::Reference);
if (ArgLoc == nullptr)
return;
Env.setStorageLocation(*S, *ArgLoc);
@@ -562,6 +542,24 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
Env.setValue(Loc, Env.getBoolLiteralValue(S->getValue()));
}
+ void VisitParenExpr(const ParenExpr *S) {
+ // The CFG does not contain `ParenExpr` as top-level statements in basic
+ // blocks, however manual traversal to sub-expressions may encounter them.
+ // Redirect to the sub-expression.
+ auto *SubExpr = S->getSubExpr();
+ assert(SubExpr != nullptr);
+ Visit(SubExpr);
+ }
+
+ void VisitExprWithCleanups(const ExprWithCleanups *S) {
+ // The CFG does not contain `ExprWithCleanups` as top-level statements in
+ // basic blocks, however manual traversal to sub-expressions may encounter
+ // them. Redirect to the sub-expression.
+ auto *SubExpr = S->getSubExpr();
+ assert(SubExpr != nullptr);
+ Visit(SubExpr);
+ }
+
private:
BoolValue &getLogicOperatorSubExprValue(const Expr &SubExpr) {
// `SubExpr` and its parent logic operator might be part of
diff erent basic
@@ -593,7 +591,6 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
};
void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env) {
- 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 cc94b8bed2f7f..a8d739c155ab6 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -46,7 +46,7 @@ class StmtToEnvMapImpl : public StmtToEnvMap {
: CFCtx(CFCtx), BlockToState(BlockToState) {}
const Environment *getEnvironment(const Stmt &S) const override {
- auto BlockIT = CFCtx.getStmtToBlock().find(&S);
+ auto BlockIT = CFCtx.getStmtToBlock().find(&ignoreCFGOmittedNodes(S));
assert(BlockIT != CFCtx.getStmtToBlock().end());
const auto &State = BlockToState[BlockIT->getSecond()->getBlockID()];
assert(State.hasValue());
@@ -77,26 +77,26 @@ class TerminatorVisitor : public ConstStmtVisitor<TerminatorVisitor> {
: StmtToEnv(StmtToEnv), Env(Env), BlockSuccIdx(BlockSuccIdx) {}
void VisitIfStmt(const IfStmt *S) {
- auto *Cond = ignoreExprWithCleanups(S->getCond())->IgnoreParens();
+ auto *Cond = S->getCond();
assert(Cond != nullptr);
extendFlowCondition(*Cond);
}
void VisitWhileStmt(const WhileStmt *S) {
- auto *Cond = ignoreExprWithCleanups(S->getCond())->IgnoreParens();
+ auto *Cond = S->getCond();
assert(Cond != nullptr);
extendFlowCondition(*Cond);
}
void VisitBinaryOperator(const BinaryOperator *S) {
assert(S->getOpcode() == BO_LAnd || S->getOpcode() == BO_LOr);
- auto *LHS = ignoreExprWithCleanups(S->getLHS())->IgnoreParens();
+ auto *LHS = S->getLHS();
assert(LHS != nullptr);
extendFlowCondition(*LHS);
}
void VisitConditionalOperator(const ConditionalOperator *S) {
- auto *Cond = ignoreExprWithCleanups(S->getCond())->IgnoreParens();
+ auto *Cond = S->getCond();
assert(Cond != nullptr);
extendFlowCondition(*Cond);
}
More information about the cfe-commits
mailing list