llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: None (martinboehme) <details> <summary>Changes</summary> Reverts llvm/llvm-project#<!-- -->87320 This is causing buildbots to fail because `isOriginalRecordConstructor()` is now unused. --- Patch is 53.11 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/88315.diff 6 Files Affected: - (modified) clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h (+20-44) - (modified) clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp (+98-307) - (modified) clang/lib/Analysis/FlowSensitive/Transfer.cpp (+98-78) - (modified) clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp (+10-3) - (modified) clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp (-43) - (modified) clang/unittests/Analysis/FlowSensitive/TransferTest.cpp (+57-115) ``````````diff diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h index 706664d7db1c25..9a65f76cdf56bc 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h @@ -30,7 +30,6 @@ #include "llvm/ADT/MapVector.h" #include "llvm/Support/Compiler.h" #include "llvm/Support/ErrorHandling.h" -#include <memory> #include <type_traits> #include <utility> @@ -345,6 +344,17 @@ class Environment { /// location of the result object to pass in `this`, even though prvalues are /// otherwise not associated with storage locations. /// + /// FIXME: Currently, this simply returns a stable storage location for `E`, + /// but this doesn't do the right thing in scenarios like the following: + /// ``` + /// MyClass c = some_condition()? MyClass(foo) : MyClass(bar); + /// ``` + /// Here, `MyClass(foo)` and `MyClass(bar)` will have two different storage + /// locations, when in fact their storage locations should be the same. + /// Eventually, we want to propagate storage locations from result objects + /// down to the prvalues that initialize them, similar to the way that this is + /// done in Clang's CodeGen. + /// /// Requirements: /// `E` must be a prvalue of record type. RecordStorageLocation & @@ -452,13 +462,7 @@ class Environment { /// Initializes the fields (including synthetic fields) of `Loc` with values, /// unless values of the field type are not supported or we hit one of the /// limits at which we stop producing values. - /// If `Type` is provided, initializes only those fields that are modeled for - /// `Type`; this is intended for use in cases where `Loc` is a derived type - /// and we only want to initialize the fields of a base type. - void initializeFieldsWithValues(RecordStorageLocation &Loc, QualType Type); - void initializeFieldsWithValues(RecordStorageLocation &Loc) { - initializeFieldsWithValues(Loc, Loc.getType()); - } + void initializeFieldsWithValues(RecordStorageLocation &Loc); /// Assigns `Val` as the value of `Loc` in the environment. void setValue(const StorageLocation &Loc, Value &Val); @@ -649,9 +653,6 @@ class Environment { LLVM_DUMP_METHOD void dump(raw_ostream &OS) const; private: - using PrValueToResultObject = - llvm::DenseMap<const Expr *, RecordStorageLocation *>; - // The copy-constructor is for use in fork() only. Environment(const Environment &) = default; @@ -681,10 +682,8 @@ class Environment { /// Initializes the fields (including synthetic fields) of `Loc` with values, /// unless values of the field type are not supported or we hit one of the /// limits at which we stop producing values (controlled by `Visited`, - /// `Depth`, and `CreatedValuesCount`). If `Type` is different from - /// `Loc.getType()`, initializes only those fields that are modeled for - /// `Type`. - void initializeFieldsWithValues(RecordStorageLocation &Loc, QualType Type, + /// `Depth`, and `CreatedValuesCount`). + void initializeFieldsWithValues(RecordStorageLocation &Loc, llvm::DenseSet<QualType> &Visited, int Depth, int &CreatedValuesCount); @@ -703,45 +702,22 @@ class Environment { /// and functions referenced in `FuncDecl`. `FuncDecl` must have a body. void initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl); - static PrValueToResultObject - buildResultObjectMap(DataflowAnalysisContext *DACtx, - const FunctionDecl *FuncDecl, - RecordStorageLocation *ThisPointeeLoc, - RecordStorageLocation *LocForRecordReturnVal); - // `DACtx` is not null and not owned by this object. DataflowAnalysisContext *DACtx; - // FIXME: move the fields `CallStack`, `ResultObjectMap`, `ReturnVal`, - // `ReturnLoc` and `ThisPointeeLoc` into a separate call-context object, - // shared between environments in the same call. + // FIXME: move the fields `CallStack`, `ReturnVal`, `ReturnLoc` and + // `ThisPointeeLoc` into a separate call-context object, shared between + // environments in the same call. // https://github.com/llvm/llvm-project/issues/59005 // `DeclContext` of the block being analysed if provided. std::vector<const DeclContext *> CallStack; - // Maps from prvalues of record type to their result objects. Shared between - // all environments for the same function. - // FIXME: It's somewhat unsatisfactory that we have to use a `shared_ptr` - // here, though the cost is acceptable: The overhead of a `shared_ptr` is - // incurred when it is copied, and this happens only relatively rarely (when - // we fork the environment). The need for a `shared_ptr` will go away once we - // introduce a shared call-context object (see above). - std::shared_ptr<PrValueToResultObject> ResultObjectMap; - - // The following three member variables handle various different types of - // return values. - // - If the return type is not a reference and not a record: Value returned - // by the function. + // Value returned by the function (if it has non-reference return type). Value *ReturnVal = nullptr; - // - If the return type is a reference: Storage location of the reference - // returned by the function. + // Storage location of the reference returned by the function (if it has + // reference return type). StorageLocation *ReturnLoc = nullptr; - // - If the return type is a record or the function being analyzed is a - // constructor: Storage location into which the return value should be - // constructed. - RecordStorageLocation *LocForRecordReturnVal = nullptr; - // The storage location of the `this` pointee. Should only be null if the // function being analyzed is only a function and not a method. RecordStorageLocation *ThisPointeeLoc = nullptr; diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp index 6c796b4ad923e8..1bfa7ebcfd50c9 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -15,7 +15,6 @@ #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" -#include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/Type.h" #include "clang/Analysis/FlowSensitive/DataflowLattice.h" #include "clang/Analysis/FlowSensitive/Value.h" @@ -27,8 +26,6 @@ #include <cassert> #include <utility> -#define DEBUG_TYPE "dataflow" - namespace clang { namespace dataflow { @@ -357,8 +354,6 @@ getFieldsGlobalsAndFuncs(const Stmt &S, FieldSet &Fields, for (auto *Child : S.children()) if (Child != nullptr) getFieldsGlobalsAndFuncs(*Child, Fields, Vars, Funcs); - if (const auto *DefaultArg = dyn_cast<CXXDefaultArgExpr>(&S)) - getFieldsGlobalsAndFuncs(*DefaultArg->getExpr(), Fields, Vars, Funcs); if (const auto *DefaultInit = dyn_cast<CXXDefaultInitExpr>(&S)) getFieldsGlobalsAndFuncs(*DefaultInit->getExpr(), Fields, Vars, Funcs); @@ -391,186 +386,6 @@ getFieldsGlobalsAndFuncs(const Stmt &S, FieldSet &Fields, } } -namespace { - -// Visitor that builds a map from record prvalues to result objects. -// This traverses the body of the function to be analyzed; for each result -// object that it encounters, it propagates the storage location of the result -// object to all record prvalues that can initialize it. -class ResultObjectVisitor : public RecursiveASTVisitor<ResultObjectVisitor> { -public: - // `ResultObjectMap` will be filled with a map from record prvalues to result - // object. If the function being analyzed returns a record by value, - // `LocForRecordReturnVal` is the location to which this record should be - // written; otherwise, it is null. - explicit ResultObjectVisitor( - llvm::DenseMap<const Expr *, RecordStorageLocation *> &ResultObjectMap, - RecordStorageLocation *LocForRecordReturnVal, - DataflowAnalysisContext &DACtx) - : ResultObjectMap(ResultObjectMap), - LocForRecordReturnVal(LocForRecordReturnVal), DACtx(DACtx) {} - - bool shouldVisitImplicitCode() { return true; } - - bool shouldVisitLambdaBody() const { return false; } - - // Traverse all member and base initializers of `Ctor`. This function is not - // called by `RecursiveASTVisitor`; it should be called manually if we are - // analyzing a constructor. `ThisPointeeLoc` is the storage location that - // `this` points to. - void TraverseConstructorInits(const CXXConstructorDecl *Ctor, - RecordStorageLocation *ThisPointeeLoc) { - assert(ThisPointeeLoc != nullptr); - for (const CXXCtorInitializer *Init : Ctor->inits()) { - Expr *InitExpr = Init->getInit(); - if (FieldDecl *Field = Init->getMember(); - Field != nullptr && Field->getType()->isRecordType()) { - PropagateResultObject(InitExpr, cast<RecordStorageLocation>( - ThisPointeeLoc->getChild(*Field))); - } else if (Init->getBaseClass()) { - PropagateResultObject(InitExpr, ThisPointeeLoc); - } - - // Ensure that any result objects within `InitExpr` (e.g. temporaries) - // are also propagated to the prvalues that initialize them. - TraverseStmt(InitExpr); - - // If this is a `CXXDefaultInitExpr`, also propagate any result objects - // within the default expression. - if (auto *DefaultInit = dyn_cast<CXXDefaultInitExpr>(InitExpr)) - TraverseStmt(DefaultInit->getExpr()); - } - } - - bool TraverseBindingDecl(BindingDecl *BD) { - // `RecursiveASTVisitor` doesn't traverse holding variables for - // `BindingDecl`s by itself, so we need to tell it to. - if (VarDecl *HoldingVar = BD->getHoldingVar()) - TraverseDecl(HoldingVar); - return RecursiveASTVisitor<ResultObjectVisitor>::TraverseBindingDecl(BD); - } - - bool VisitVarDecl(VarDecl *VD) { - if (VD->getType()->isRecordType() && VD->hasInit()) - PropagateResultObject( - VD->getInit(), - &cast<RecordStorageLocation>(DACtx.getStableStorageLocation(*VD))); - return true; - } - - bool VisitMaterializeTemporaryExpr(MaterializeTemporaryExpr *MTE) { - if (MTE->getType()->isRecordType()) - PropagateResultObject( - MTE->getSubExpr(), - &cast<RecordStorageLocation>(DACtx.getStableStorageLocation(*MTE))); - return true; - } - - bool VisitReturnStmt(ReturnStmt *Return) { - Expr *RetValue = Return->getRetValue(); - if (RetValue != nullptr && RetValue->getType()->isRecordType() && - RetValue->isPRValue()) - PropagateResultObject(RetValue, LocForRecordReturnVal); - return true; - } - - bool VisitExpr(Expr *E) { - // Clang's AST can have record-type prvalues without a result object -- for - // example as full-expressions contained in a compound statement or as - // arguments of call expressions. We notice this if we get here and a - // storage location has not yet been associated with `E`. In this case, - // treat this as if it was a `MaterializeTemporaryExpr`. - if (E->isPRValue() && E->getType()->isRecordType() && - !ResultObjectMap.contains(E)) - PropagateResultObject( - E, &cast<RecordStorageLocation>(DACtx.getStableStorageLocation(*E))); - return true; - } - - // Assigns `Loc` as the result object location of `E`, then propagates the - // location to all lower-level prvalues that initialize the same object as - // `E` (or one of its base classes or member variables). - void PropagateResultObject(Expr *E, RecordStorageLocation *Loc) { - if (!E->isPRValue() || !E->getType()->isRecordType()) { - assert(false); - // Ensure we don't propagate the result object if we hit this in a - // release build. - return; - } - - ResultObjectMap[E] = Loc; - - // The following AST node kinds are "original initializers": They are the - // lowest-level AST node that initializes a given object, and nothing - // below them can initialize the same object (or part of it). - if (isa<CXXConstructExpr>(E) || isa<CallExpr>(E) || isa<LambdaExpr>(E) || - isa<CXXDefaultArgExpr>(E) || isa<CXXDefaultInitExpr>(E) || - isa<CXXStdInitializerListExpr>(E)) { - return; - } - - if (auto *InitList = dyn_cast<InitListExpr>(E)) { - if (!InitList->isSemanticForm()) - return; - if (InitList->isTransparent()) { - PropagateResultObject(InitList->getInit(0), Loc); - return; - } - - RecordInitListHelper InitListHelper(InitList); - - for (auto [Base, Init] : InitListHelper.base_inits()) { - assert(Base->getType().getCanonicalType() == - Init->getType().getCanonicalType()); - - // Storage location for the base class is the same as that of the - // derived class because we "flatten" the object hierarchy and put all - // fields in `RecordStorageLocation` of the derived class. - PropagateResultObject(Init, Loc); - } - - for (auto [Field, Init] : InitListHelper.field_inits()) { - // Fields of non-record type are handled in - // `TransferVisitor::VisitInitListExpr()`. - if (!Field->getType()->isRecordType()) - continue; - PropagateResultObject( - Init, cast<RecordStorageLocation>(Loc->getChild(*Field))); - } - return; - } - - if (auto *Op = dyn_cast<BinaryOperator>(E); Op && Op->isCommaOp()) { - PropagateResultObject(Op->getRHS(), Loc); - return; - } - - if (auto *Cond = dyn_cast<AbstractConditionalOperator>(E)) { - PropagateResultObject(Cond->getTrueExpr(), Loc); - PropagateResultObject(Cond->getFalseExpr(), Loc); - return; - } - - // All other expression nodes that propagate a record prvalue should have - // exactly one child. - SmallVector<Stmt *, 1> Children(E->child_begin(), E->child_end()); - LLVM_DEBUG({ - if (Children.size() != 1) - E->dump(); - }); - assert(Children.size() == 1); - for (Stmt *S : Children) - PropagateResultObject(cast<Expr>(S), Loc); - } - -private: - llvm::DenseMap<const Expr *, RecordStorageLocation *> &ResultObjectMap; - RecordStorageLocation *LocForRecordReturnVal; - DataflowAnalysisContext &DACtx; -}; - -} // namespace - Environment::Environment(DataflowAnalysisContext &DACtx) : DACtx(&DACtx), FlowConditionToken(DACtx.arena().makeFlowConditionToken()) {} @@ -586,23 +401,17 @@ void Environment::initialize() { if (DeclCtx == nullptr) return; - const auto *FuncDecl = dyn_cast<FunctionDecl>(DeclCtx); - if (FuncDecl == nullptr) - return; - - assert(FuncDecl->doesThisDeclarationHaveABody()); + if (const auto *FuncDecl = dyn_cast<FunctionDecl>(DeclCtx)) { + assert(FuncDecl->doesThisDeclarationHaveABody()); - initFieldsGlobalsAndFuncs(FuncDecl); + initFieldsGlobalsAndFuncs(FuncDecl); - for (const auto *ParamDecl : FuncDecl->parameters()) { - assert(ParamDecl != nullptr); - setStorageLocation(*ParamDecl, createObject(*ParamDecl, nullptr)); + for (const auto *ParamDecl : FuncDecl->parameters()) { + assert(ParamDecl != nullptr); + setStorageLocation(*ParamDecl, createObject(*ParamDecl, nullptr)); + } } - if (FuncDecl->getReturnType()->isRecordType()) - LocForRecordReturnVal = &cast<RecordStorageLocation>( - createStorageLocation(FuncDecl->getReturnType())); - if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(DeclCtx)) { auto *Parent = MethodDecl->getParent(); assert(Parent != nullptr); @@ -635,12 +444,6 @@ void Environment::initialize() { initializeFieldsWithValues(ThisLoc); } } - - // We do this below the handling of `CXXMethodDecl` above so that we can - // be sure that the storage location for `this` has been set. - ResultObjectMap = std::make_shared<PrValueToResultObject>( - buildResultObjectMap(DACtx, FuncDecl, getThisPointeeStorageLocation(), - LocForRecordReturnVal)); } // FIXME: Add support for resetting globals after function calls to enable @@ -681,18 +484,13 @@ void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) { if (getStorageLocation(*D) != nullptr) continue; - // We don't run transfer functions on the initializers of global variables, - // so they won't be associated with a value or storage location. We - // therefore intentionally don't pass an initializer to `createObject()`; - // in particular, this ensures that `createObject()` will initialize the - // fields of record-type variables with values. - setStorageLocation(*D, createObject(*D, nullptr)); + setStorageLocation(*D, createObject(*D)); } for (const FunctionDecl *FD : Funcs) { if (getStorageLocation(*FD) != nullptr) continue; - auto &Loc = createStorageLocation(*FD); + auto &Loc = createStorageLocation(FD->getType()); setStorageLocation(*FD, Loc); } } @@ -721,9 +519,6 @@ Environment Environment::pushCall(const CallExpr *Call) const { } } - if (Call->getType()->isRecordType() && Call->isPRValue()) - Env.LocForRecordReturnVal = &Env.getResultObjectLocation(*Call); - Env.pushCallInternal(Call->getDirectCallee(), llvm::ArrayRef(Call->getArgs(), Call->getNumArgs())); @@ -734,7 +529,6 @@ Environment Environment::pushCall(const CXXConstructExpr *Call) const { Environment Env(*this); Env.ThisPointeeLoc = &Env.getResultObjectLocation(*Call); - Env.LocForRecordReturnVal = &Env.getResultObjectLocation(*Call); Env.pushCallInternal(Call->getConstructor(), llvm::ArrayRef(Call->getArgs(), Call->getNumArgs())); @@ -763,10 +557,6 @@ void Environment::pushCallInternal(const FunctionDecl *FuncDecl, const VarDecl *Param = *ParamIt; setStorageLocation(*Param, createObject(*Param, Args[ArgIndex])); } - - ResultObjectMap = std::make_shared<PrValueToResultObject>( - buildResultObjectMap(DACtx, FuncDecl, getThisPointeeStorageLocation(), - LocForRecordReturnVal)); } void Environment::popCall(const CallExpr *Call, const Environment &CalleeEnv) { @@ -810,9 +600,6 @@ bool Environment::equivalentTo(const Environment &Other, if (ReturnLoc != Other.ReturnLoc) return false; - if (LocForRecordReturnVal != Other.LocForRecordReturnVal) - return false; - if (ThisPointeeLoc != Other.ThisPointeeLoc) return false; @@ -836,10 +623,8 @@ LatticeEffect Environment::widen(const Environment &PrevEnv, assert(DACtx == PrevEnv.DACtx); assert(ReturnVal == PrevEnv.ReturnVal); assert(ReturnLoc == PrevEnv.ReturnLoc); - assert(LocForRecordReturnVal == PrevEnv.LocForRecordReturnVal); assert(ThisPointeeLoc == PrevEnv.ThisPointeeLoc); assert(CallStack == PrevEnv.CallStack); - assert(ResultObjectMap == PrevEnv.ResultObjectMap); auto Effect = LatticeEffect::Unchanged; @@ -871,16 +656,12 @@ Environment Environment::join(const Environment &EnvA, const Environment &EnvB, Environment::ValueModel &Model, ExprJoinBehavior ExprBehavior) { assert(EnvA.DACtx == EnvB.DACtx); - assert(EnvA.LocForRecordReturnVal == EnvB.LocForRecordReturnVal); assert(EnvA.ThisPointeeLoc == EnvB.ThisPointeeLoc); assert(EnvA.CallStack == EnvB.CallStack); - assert(EnvA.ResultObjectMap == EnvB.ResultObjectMap); Environment JoinedEnv(*EnvA.DACtx); JoinedEnv.CallStack = EnvA.CallStack; - JoinedEnv.ResultObjectMap = EnvA.ResultObjectMap; - JoinedEnv.LocForRecordReturnVal = EnvA.LocForRecordReturnVal; JoinedEnv.ThisPointeeLoc = EnvA.ThisPointeeLoc; if (EnvA.ReturnVal == nullptr || EnvB.ReturnVal == nullptr) { @@ -949,12 +730,6 @@ StorageLocation &Environment::createStorageLocation(const Expr &E) { void Environment::setStorageLocation(const ValueDecl &D, StorageLocation &Loc) { assert(!DeclToLoc.contains(&D)); - // The only kinds of declarations that may have a "variable" storage location - // are declarations of reference type and `BindingDecl`. For all other - // declaration, the storage location should be the stable storage location - ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/88315 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
