Author: Martin Braenne Date: 2023-05-17T09:30:47Z New Revision: 080ee850c639be96b8bea8008088f2b036ff0f15
URL: https://github.com/llvm/llvm-project/commit/080ee850c639be96b8bea8008088f2b036ff0f15 DIFF: https://github.com/llvm/llvm-project/commit/080ee850c639be96b8bea8008088f2b036ff0f15.diff LOG: [clang][dataflow] Add `Strict` versions of `Value` and `StorageLocation` accessors. This is part of the gradual migration to strict handling of value categories, as described in the RFC at https://discourse.llvm.org/t/70086. This patch migrates some representative calls of the newly deprecated accessors to the new `Strict` functions. Followup patches will migrate the remaining callers. (There are a large number of callers, with some subtlety involved in some of them, so it makes sense to split this up into multiple patches rather than migrating all callers in one go.) The `Strict` accessors as implemented here have some differences in semantics compared to the semantics originally proposed in the RFC; specifically: * `setStorageLocationStrict()`: The RFC proposes to create an intermediate `ReferenceValue` that then refers to the `StorageLocation` for the glvalue. It turns out though that, even today, most places in the code are not doing this but are instead associating glvalues directly with their `StorageLocation`. It therefore didn't seem to make sense to introduce new `ReferenceValue`s where there were none previously, so I have chosen to instead make `setStorageLocationStrict()` simply call through to `setStorageLocation(const Expr &, StorageLocation &)` and merely add the assertion that the expression must be a glvalue. * `getStorageLocationStrict()`: The RFC proposes that this should assert that the storage location for the glvalue expression is associated with an intermediate `ReferenceValue`, but, as explained, this is often not true. The current state is inconsistent: Sometimes the intermediate `ReferenceValue` is there, sometimes it isn't. For this reason, `getStorageLocationStrict()` skips past a `ReferenceValue` if it is there but otherwise directly returns the storage location associated with the expression. This behavior is equivalent to the existing behavior of `SkipPast::Reference`. * `setValueStrict()`: The RFC proposes that this should always create the same `StorageLocation` for a given `Value`, but, in fact, the transfer functions that exist today don't guarantee this; almost all transfer functions unconditionally create a new `StorageLocation` when associating an expression with a `Value`. There appears to be one special case: `TerminatorVisitor::extendFlowCondition()` checks whether the expression is already associated with a `StorageLocation` and, if so, reuses the existing `StorageLocation` instead of creating a new one. For this reason, `setValueStrict()` implements this logic (preserve an existing `StorageLocation`) but makes no attempt to always associate the same `StorageLocation` with a given `Value`, as nothing in the framework appers to require this. As `TerminatorVisitor::extendFlowCondition()` is an interesting special case, the `setValue()` call there is among the ones that this patch migrates to `setValueStrict()`. Reviewed By: sammccall, ymandel, xazax.hun Differential Revision: https://reviews.llvm.org/D150653 Added: Modified: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp clang/lib/Analysis/FlowSensitive/Transfer.cpp clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h index d734ae5c66fe8..6f2f7f4004f84 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h @@ -270,16 +270,54 @@ class Environment { /// 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 @@ class Environment { /// 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 @@ class Environment { /// 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 index 7b1944f273cf0..aaa2f6f19eecb 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -623,6 +623,16 @@ void Environment::setStorageLocation(const Expr &E, StorageLocation &Loc) { 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 @@ StorageLocation *Environment::getStorageLocation(const Expr &E, 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<ReferenceValue>(getValue(*Loc))) + return &RefVal->getReferentLoc(); + + return Loc; +} + StorageLocation *Environment::getThisPointeeStorageLocation() const { return ThisPointeeLoc; } @@ -675,6 +700,18 @@ void Environment::setValue(const StorageLocation &Loc, Value &Val) { } } +void Environment::setValueStrict(const Expr &E, Value &Val) { + assert(E.isPRValue()); + assert(!isa<ReferenceValue>(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 @@ Value *Environment::getValue(const Expr &E, SkipPast SP) const { return getValue(*Loc); } +Value *Environment::getValueStrict(const Expr &E) const { + assert(E.isPRValue()); + Value *Val = getValue(E, SkipPast::None); + + assert(Val == nullptr || !isa<ReferenceValue>(Val)); + + return Val; +} + Value *Environment::createValue(QualType Type) { llvm::DenseSet<QualType> Visited; int CreatedValuesCount = 0; diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp index 95810a47fc632..45c601ed49146 100644 --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -223,7 +223,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> { if (DeclLoc == nullptr) return; - Env.setStorageLocation(*S, *DeclLoc); + Env.setStorageLocationStrict(*S, *DeclLoc); } void VisitDeclStmt(const DeclStmt *S) { @@ -343,15 +343,13 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> { // 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<BoolValue>( - Env.getValue(*SubExpr, SkipPast::Reference))) - Env.setValue(Loc, *SubExprVal); + if (auto *SubExprVal = + dyn_cast_or_null<BoolValue>(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 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> { switch (S->getOpcode()) { case UO_Deref: { const auto *SubExprVal = - cast_or_null<PointerValue>(Env.getValue(*SubExpr, SkipPast::None)); + cast_or_null<PointerValue>(Env.getValueStrict(*SubExpr)); if (SubExprVal == nullptr) break; - auto &Loc = Env.createStorageLocation(*S); - Env.setStorageLocation(*S, Loc); - Env.setValue(Loc, - Env.create<ReferenceValue>(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<PointerValue>(*PointeeLoc); - Env.setStorageLocation(*S, PointerLoc); - Env.setValue(PointerLoc, PointerVal); + Env.setValueStrict(*S, Env.create<PointerValue>(*PointeeLoc)); break; } case UO_LNot: { diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp index 418a6cff21cc1..2e9893e78fa7d 100644 --- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp +++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp @@ -135,14 +135,8 @@ class TerminatorVisitor // 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; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
