li.zhe.hua created this revision.
li.zhe.hua added reviewers: sgatev, ymandel.
Herald added subscribers: tschuett, steakhal.
Herald added a project: All.
li.zhe.hua requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
A follow-up to 62b2a47
<https://reviews.llvm.org/rG62b2a47a9f15ed2f1dc4b39c924341c7b9bd7cf8> 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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D124965
Files:
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
Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -46,7 +46,7 @@
: 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 @@
: 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);
}
Index: clang/lib/Analysis/FlowSensitive/Transfer.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ 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 @@
: 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 @@
return;
}
- InitExpr = ignoreExprWithCleanups(D.getInit());
+ InitExpr = D.getInit();
assert(InitExpr != nullptr);
if (D.getType()->isReferenceType()) {
@@ -476,8 +457,7 @@
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 @@
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 different basic
@@ -593,7 +591,6 @@
};
void transfer(const StmtToEnvMap &StmtToEnv, const Stmt &S, Environment &Env) {
- assert(!(isa<ParenExpr, ExprWithCleanups>(&S)));
TransferVisitor(StmtToEnv, Env).Visit(&S);
}
Index: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
+++ clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
@@ -343,7 +343,6 @@
}
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,14 @@
}
void Environment::setStorageLocation(const Expr &E, StorageLocation &Loc) {
- assert(!isa<ExprWithCleanups>(&E));
- assert(ExprToLoc.find(&E) == ExprToLoc.end());
- ExprToLoc[&E] = &Loc;
+ assert(ExprToLoc.find(&ignoreCFGOmittedNodes(E)) == ExprToLoc.end());
+ ExprToLoc[&ignoreCFGOmittedNodes(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());
+ auto It = ExprToLoc.find(&ignoreCFGOmittedNodes(E));
return It == ExprToLoc.end() ? nullptr : &skip(*It->second, SP);
}
Index: clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp
+++ 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>
@@ -142,3 +143,22 @@
} // 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;
+}
Index: clang/include/clang/Analysis/FlowSensitive/Transfer.h
===================================================================
--- clang/include/clang/Analysis/FlowSensitive/Transfer.h
+++ clang/include/clang/Analysis/FlowSensitive/Transfer.h
@@ -38,16 +38,6 @@
/// `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
Index: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
===================================================================
--- clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
+++ clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
@@ -172,10 +172,6 @@
/// 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 @@
/// 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 @@
/// 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
Index: clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
===================================================================
--- clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
+++ 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,14 @@
///
/// `E` must not be assigned a storage location.
void setStorageLocation(const Expr &E, StorageLocation &Loc) {
- assert(ExprToLoc.find(&E) == ExprToLoc.end());
- ExprToLoc[&E] = &Loc;
+ assert(ExprToLoc.find(&ignoreCFGOmittedNodes(E)) == ExprToLoc.end());
+ ExprToLoc[&ignoreCFGOmittedNodes(E)] = &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;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
