diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h --- 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,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; } diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/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 diff --git a/clang/include/clang/Analysis/FlowSensitive/Transfer.h b/clang/include/clang/Analysis/FlowSensitive/Transfer.h --- a/clang/include/clang/Analysis/FlowSensitive/Transfer.h +++ b/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 diff --git a/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp --- 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 #include @@ -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(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(&S)) + return ignoreCFGOmittedNodes(*E); + return S; +} diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -343,7 +343,6 @@ } StorageLocation &Environment::createStorageLocation(const Expr &E) { - assert(!isa(&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(&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(&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 --- 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(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( - Env.getValue(*LHSNorm, SkipPast::Reference))) - if (auto *RHSValue = dyn_cast_or_null( - Env.getValue(*RHSNorm, SkipPast::Reference))) + if (auto *LHSValue = + dyn_cast_or_null(Env.getValue(LHS, SkipPast::Reference))) + if (auto *RHSValue = + dyn_cast_or_null(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(&S))); TransferVisitor(StmtToEnv, Env).Visit(&S); } diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp --- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp +++ b/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); }