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 @@ -270,16 +270,54 @@ /// Assigns `Loc` as the storage location of `E` in the environment. /// + /// This function is deprecated; prefer `setStorageLocationStrict()`. + /// For details, see https://discourse.llvm.org/t/70086. + /// /// Requirements: /// /// `E` must not be assigned a storage location in the environment. void setStorageLocation(const Expr &E, StorageLocation &Loc); + /// Assigns `Loc` as the storage location of the glvalue `E` in the + /// environment. + /// + /// This function is the preferred alternative to + /// `setStorageLocation(const Expr &, StorageLocation &)`. Once the migration + /// to strict handling of value categories is complete (see + /// https://discourse.llvm.org/t/70086), `setStorageLocation()` will be + /// removed and this function will be renamed to `setStorageLocation()`. + /// + /// Requirements: + /// + /// `E` must not be assigned a storage location in the environment. + /// `E` must be a glvalue or a `BuiltinType::BuiltinFn` + void setStorageLocationStrict(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. + /// + /// This function is deprecated; prefer `getStorageLocationStrict()`. + /// For details, see https://discourse.llvm.org/t/70086. StorageLocation *getStorageLocation(const Expr &E, SkipPast SP) const; + /// Returns the storage location assigned to the glvalue `E` in the + /// environment, or null if `E` isn't assigned a storage location in the + /// environment. + /// + /// If the storage location for `E` is associated with a + /// `ReferenceValue RefVal`, returns `RefVal.getReferentLoc()` instead. + /// + /// This function is the preferred alternative to + /// `getStorageLocation(const Expr &, SkipPast)`. Once the migration + /// to strict handling of value categories is complete (see + /// https://discourse.llvm.org/t/70086), `getStorageLocation()` will be + /// removed and this function will be renamed to `getStorageLocation()`. + /// + /// Requirements: + /// `E` must be a glvalue or a `BuiltinType::BuiltinFn` + StorageLocation *getStorageLocationStrict(const Expr &E) const; + /// Returns the storage location assigned to the `this` pointee in the /// environment or null if the `this` pointee has no assigned storage location /// in the environment. @@ -305,6 +343,25 @@ /// Assigns `Val` as the value of `Loc` in the environment. void setValue(const StorageLocation &Loc, Value &Val); + /// Assigns `Val` as the value of the prvalue `E` in the environment. + /// + /// If `E` is not yet associated with a storage location, associates it with + /// a newly created storage location. In any case, associates the storage + /// location of `E` with `Val`. + /// + /// Once the migration to strict handling of value categories is complete + /// (see https://discourse.llvm.org/t/70086), this function will be renamed to + /// `setValue()`. At this point, prvalue expressions will be associated + /// directly with `Value`s, and the legacy behavior of associating prvalue + /// expressions with storage locations (as described above) will be + /// eliminated. + /// + /// Requirements: + /// + /// `E` must be a prvalue + /// `Val` must not be a `ReferenceValue` + void setValueStrict(const Expr &E, Value &Val); + /// Returns the value assigned to `Loc` in the environment or null if `Loc` /// isn't assigned a value in the environment. Value *getValue(const StorageLocation &Loc) const; @@ -315,8 +372,25 @@ /// Equivalent to `getValue(getStorageLocation(E, SP), SkipPast::None)` if `E` /// is assigned a storage location in the environment, otherwise returns null. + /// + /// This function is deprecated; prefer `getValueStrict()`. For details, see + /// https://discourse.llvm.org/t/70086. Value *getValue(const Expr &E, SkipPast SP) const; + /// Returns the `Value` assigned to the prvalue `E` in the environment, or + /// null if `E` isn't assigned a value in the environment. + /// + /// This function is the preferred alternative to + /// `getValue(const Expr &, SkipPast)`. Once the migration to strict handling + /// of value categories is complete (see https://discourse.llvm.org/t/70086), + /// `getValue()` will be removed and this function will be renamed to + /// `getValue()`. + /// + /// Requirements: + /// + /// `E` must be a prvalue + Value *getValueStrict(const Expr &E) const; + // FIXME: should we deprecate the following & call arena().create() directly? /// Creates a `T` (some subclass of `Value`), forwarding `args` to the 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 @@ -623,6 +623,16 @@ ExprToLoc[&CanonE] = &Loc; } +void Environment::setStorageLocationStrict(const Expr &E, + StorageLocation &Loc) { + // `DeclRefExpr`s to builtin function types aren't glvalues, for some reason, + // but we still want to be able to associate a `StorageLocation` with them, + // so allow these as an exception. + assert(E.isGLValue() || + E.getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn)); + setStorageLocation(E, Loc); +} + StorageLocation *Environment::getStorageLocation(const Expr &E, SkipPast SP) const { // FIXME: Add a test with parens. @@ -630,6 +640,21 @@ return It == ExprToLoc.end() ? nullptr : &skip(*It->second, SP); } +StorageLocation *Environment::getStorageLocationStrict(const Expr &E) const { + // See comment in `setStorageLocationStrict()`. + assert(E.isGLValue() || + E.getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn)); + StorageLocation *Loc = getStorageLocation(E, SkipPast::None); + + if (Loc == nullptr) + return nullptr; + + if (auto *RefVal = dyn_cast_or_null(getValue(*Loc))) + return &RefVal->getReferentLoc(); + + return Loc; +} + StorageLocation *Environment::getThisPointeeStorageLocation() const { return ThisPointeeLoc; } @@ -675,6 +700,18 @@ } } +void Environment::setValueStrict(const Expr &E, Value &Val) { + assert(E.isPRValue()); + assert(!isa(Val)); + + StorageLocation *Loc = getStorageLocation(E, SkipPast::None); + if (Loc == nullptr) { + Loc = &createStorageLocation(E); + setStorageLocation(E, *Loc); + } + setValue(*Loc, Val); +} + Value *Environment::getValue(const StorageLocation &Loc) const { auto It = LocToVal.find(&Loc); return It == LocToVal.end() ? nullptr : It->second; @@ -694,6 +731,15 @@ return getValue(*Loc); } +Value *Environment::getValueStrict(const Expr &E) const { + assert(E.isPRValue()); + Value *Val = getValue(E, SkipPast::None); + + assert(Val == nullptr || !isa(Val)); + + return Val; +} + Value *Environment::createValue(QualType Type) { llvm::DenseSet Visited; int CreatedValuesCount = 0; 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 @@ -223,7 +223,7 @@ if (DeclLoc == nullptr) return; - Env.setStorageLocation(*S, *DeclLoc); + Env.setStorageLocationStrict(*S, *DeclLoc); } void VisitDeclStmt(const DeclStmt *S) { @@ -343,15 +343,13 @@ // This cast creates a new, boolean value from the integral value. We // model that with a fresh value in the environment, unless it's already a // boolean. - auto &Loc = Env.createStorageLocation(*S); - Env.setStorageLocation(*S, Loc); - if (auto *SubExprVal = dyn_cast_or_null( - Env.getValue(*SubExpr, SkipPast::Reference))) - Env.setValue(Loc, *SubExprVal); + if (auto *SubExprVal = + dyn_cast_or_null(Env.getValueStrict(*SubExpr))) + Env.setValueStrict(*S, *SubExprVal); else // FIXME: If integer modeling is added, then update this code to create // the boolean based on the integer model. - Env.setValue(Loc, Env.makeAtomicBoolValue()); + Env.setValueStrict(*S, Env.makeAtomicBoolValue()); break; } @@ -425,29 +423,22 @@ switch (S->getOpcode()) { case UO_Deref: { const auto *SubExprVal = - cast_or_null(Env.getValue(*SubExpr, SkipPast::None)); + cast_or_null(Env.getValueStrict(*SubExpr)); if (SubExprVal == nullptr) break; - auto &Loc = Env.createStorageLocation(*S); - Env.setStorageLocation(*S, Loc); - Env.setValue(Loc, - Env.create(SubExprVal->getPointeeLoc())); + Env.setStorageLocationStrict(*S, SubExprVal->getPointeeLoc()); break; } case UO_AddrOf: { // Do not form a pointer to a reference. If `SubExpr` is assigned a // `ReferenceValue` then form a value that points to the location of its // pointee. - StorageLocation *PointeeLoc = - Env.getStorageLocation(*SubExpr, SkipPast::Reference); + StorageLocation *PointeeLoc = Env.getStorageLocationStrict(*SubExpr); if (PointeeLoc == nullptr) break; - auto &PointerLoc = Env.createStorageLocation(*S); - auto &PointerVal = Env.create(*PointeeLoc); - Env.setStorageLocation(*S, PointerLoc); - Env.setValue(PointerLoc, PointerVal); + Env.setValueStrict(*S, Env.create(*PointeeLoc)); break; } case UO_LNot: { 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 @@ -135,14 +135,8 @@ // entirety (even if they may share some clauses). So, we need *some* value // for the condition expression, even if just an atom. if (Val == nullptr) { - // FIXME: Consider introducing a helper for this get-or-create pattern. - auto *Loc = Env.getStorageLocation(Cond, SkipPast::None); - if (Loc == nullptr) { - Loc = &Env.createStorageLocation(Cond); - Env.setStorageLocation(Cond, *Loc); - } Val = &Env.makeAtomicBoolValue(); - Env.setValue(*Loc, *Val); + Env.setValueStrict(Cond, *Val); } bool ConditionValue = true;