This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG45643cfcc12e: [clang][dataflow] Centralize expression
skipping logic (authored by li.zhe.hua).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124965/new/
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,15 @@
}
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);
}
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>
@@ -155,3 +156,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,15 @@
///
/// `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;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
