diff options
author | martinboehme <mboehme@google.com> | 2023-12-04 09:29:22 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-12-04 09:29:22 +0100 |
commit | 71f2ec2db1295462d61e1407fcc1e715ba5d458b (patch) | |
tree | 00bc11adc1a025b59007f5bcd3013c91e350a861 /clang | |
parent | 0cdacd5f492991362bfc8e252673aafdb9651322 (diff) | |
download | llvm-71f2ec2db1295462d61e1407fcc1e715ba5d458b.zip llvm-71f2ec2db1295462d61e1407fcc1e715ba5d458b.tar.gz llvm-71f2ec2db1295462d61e1407fcc1e715ba5d458b.tar.bz2 |
[clang][dataflow] Add synthetic fields to `RecordStorageLocation` (#73860)
Synthetic fields are intended to model the internal state of a class
(e.g. the value stored in a `std::optional`) without having to depend on
that class's implementation details.
Today, this is typically done with properties on `RecordValue`s, but
these have several drawbacks:
* Care must be taken to call `refreshRecordValue()` before modifying a
property so that the modified property values aren’t seen by other
environments that may have access to the same `RecordValue`.
* Properties aren’t associated with a storage location. If an analysis
needs to associate a location with the value stored in a property (e.g.
to model the reference returned by `std::optional::value()`), it needs
to manually add an indirection using a `PointerValue`. (See for example
the way this is done in UncheckedOptionalAccessModel.cpp, specifically
in `maybeInitializeOptionalValueMember()`.)
* Properties don’t participate in the builtin compare, join, and widen
operations. If an analysis needs to apply these operations to
properties, it needs to override the corresponding methods of
`ValueModel`.
* Longer-term, we plan to eliminate `RecordValue`, as by-value
operations on records aren’t really “a thing” in C++ (see
https://discourse.llvm.org/t/70086#changed-structvalue-api-14). This
would obviously eliminate the ability to set properties on
`RecordValue`s.
To demonstrate the advantages of synthetic fields, this patch converts
UncheckedOptionalAccessModel.cpp to synthetic fields. This greatly
simplifies the implementation of the check.
This PR is pretty big; to make it easier to review, I have broken it
down into a stack of three commits, each of which contains a set of
logically related changes. I considered submitting each of these as a
separate PR, but the commits only really make sense when taken together.
To review, I suggest first looking at the changes in
UncheckedOptionalAccessModel.cpp. This gives a flavor for how the
various API changes work together in the context of an analysis. Then,
review the rest of the changes.
Diffstat (limited to 'clang')
18 files changed, 448 insertions, 384 deletions
diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h index c5f14cf..1dffbe8 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h @@ -234,6 +234,22 @@ runDataflowAnalysis( return std::move(BlockStates); } +// Create an analysis class that is derived from `DataflowAnalysis`. This is an +// SFINAE adapter that allows us to call two different variants of constructor +// (either with or without the optional `Environment` parameter). +// FIXME: Make all classes derived from `DataflowAnalysis` take an `Environment` +// parameter in their constructor so that we can get rid of this abomination. +template <typename AnalysisT> +auto createAnalysis(ASTContext &ASTCtx, Environment &Env) + -> decltype(AnalysisT(ASTCtx, Env)) { + return AnalysisT(ASTCtx, Env); +} +template <typename AnalysisT> +auto createAnalysis(ASTContext &ASTCtx, Environment &Env) + -> decltype(AnalysisT(ASTCtx)) { + return AnalysisT(ASTCtx); +} + /// Runs a dataflow analysis over the given function and then runs `Diagnoser` /// over the results. Returns a list of diagnostics for `FuncDecl` or an /// error. Currently, errors can occur (at least) because the analysis requires @@ -262,7 +278,7 @@ llvm::Expected<llvm::SmallVector<Diagnostic>> diagnoseFunction( const WatchedLiteralsSolver *Solver = OwnedSolver.get(); DataflowAnalysisContext AnalysisContext(std::move(OwnedSolver)); Environment Env(AnalysisContext, FuncDecl); - AnalysisT Analysis(ASTCtx); + AnalysisT Analysis = createAnalysis<AnalysisT>(ASTCtx, Env); llvm::SmallVector<Diagnostic> Diagnostics; if (llvm::Error Err = runTypeErasedDataflowAnalysis( diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h index c1aa848..20e45cc 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h @@ -58,6 +58,10 @@ using FieldSet = llvm::SmallSetVector<const FieldDecl *, 4>; /// Returns the set of all fields in the type. FieldSet getObjectFields(QualType Type); +/// Returns whether `Fields` and `FieldLocs` contain the same fields. +bool containsSameFields(const FieldSet &Fields, + const RecordStorageLocation::FieldToLoc &FieldLocs); + struct ContextSensitiveOptions { /// The maximum depth to analyze. A value of zero is equivalent to disabling /// context-sensitive analysis entirely. @@ -92,11 +96,39 @@ public: /*Logger=*/nullptr}); ~DataflowAnalysisContext(); + /// Sets a callback that returns the names and types of the synthetic fields + /// to add to a `RecordStorageLocation` of a given type. + /// Typically, this is called from the constructor of a `DataflowAnalysis` + /// + /// To maintain the invariant that all `RecordStorageLocation`s of a given + /// type have the same fields: + /// * The callback must always return the same result for a given type + /// * `setSyntheticFieldCallback()` must be called before any + // `RecordStorageLocation`s are created. + void setSyntheticFieldCallback( + std::function<llvm::StringMap<QualType>(QualType)> CB) { + assert(!RecordStorageLocationCreated); + SyntheticFieldCallback = CB; + } + /// Returns a new storage location appropriate for `Type`. /// /// A null `Type` is interpreted as the pointee type of `std::nullptr_t`. StorageLocation &createStorageLocation(QualType Type); + /// Creates a `RecordStorageLocation` for the given type and with the given + /// fields. + /// + /// Requirements: + /// + /// `FieldLocs` must contain exactly the fields returned by + /// `getModeledFields(Type)`. + /// `SyntheticFields` must contain exactly the fields returned by + /// `getSyntheticFields(Type)`. + RecordStorageLocation &createRecordStorageLocation( + QualType Type, RecordStorageLocation::FieldToLoc FieldLocs, + RecordStorageLocation::SyntheticFieldMap SyntheticFields); + /// Returns a stable storage location for `D`. StorageLocation &getStableStorageLocation(const ValueDecl &D); @@ -169,6 +201,15 @@ public: /// context. FieldSet getModeledFields(QualType Type); + /// Returns the names and types of the synthetic fields for the given record + /// type. + llvm::StringMap<QualType> getSyntheticFields(QualType Type) { + assert(Type->isRecordType()); + if (SyntheticFieldCallback) + return SyntheticFieldCallback(Type); + return {}; + } + private: friend class Environment; @@ -250,6 +291,11 @@ private: FieldSet ModeledFields; std::unique_ptr<Logger> LogOwner; // If created via flags. + + std::function<llvm::StringMap<QualType>(QualType)> SyntheticFieldCallback; + + /// Has any `RecordStorageLocation` been created yet? + bool RecordStorageLocationCreated = false; }; } // namespace dataflow diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h index 7c1f549..d7e39ab 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h @@ -166,6 +166,15 @@ public: /// with a symbolic representation of the `this` pointee. Environment(DataflowAnalysisContext &DACtx, const DeclContext &DeclCtx); + /// Assigns storage locations and values to all parameters, captures, global + /// variables, fields and functions referenced in the function currently being + /// analyzed. + /// + /// Requirements: + /// + /// The function must have a body. + void initialize(); + /// Returns a new environment that is a copy of this one. /// /// The state of the program is initially the same, but can be mutated without @@ -283,7 +292,15 @@ public: /// 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. - RecordStorageLocation *getThisPointeeStorageLocation() const; + RecordStorageLocation *getThisPointeeStorageLocation() const { + return ThisPointeeLoc; + } + + /// Sets the storage location assigned to the `this` pointee in the + /// environment. + void setThisPointeeStorageLocation(RecordStorageLocation &Loc) { + ThisPointeeLoc = &Loc; + } /// Returns the location of the result object for a record-type prvalue. /// @@ -367,7 +384,8 @@ public: /// storage locations and values for indirections until it finds a /// non-pointer/non-reference type. /// - /// If `Type` is a class, struct, or union type, calls `setValue()` to + /// If `Type` is a class, struct, or union type, creates values for all + /// modeled fields (including synthetic fields) and calls `setValue()` to /// associate the `RecordValue` with its storage location /// (`RecordValue::getLoc()`). /// @@ -570,6 +588,9 @@ public: return dyn_cast<FunctionDecl>(getDeclCtx()); } + /// Returns the size of the call stack. + size_t callStackSize() const { return CallStack.size(); } + /// Returns whether this `Environment` can be extended to analyze the given /// `Callee` (i.e. if `pushCall` can be used), with recursion disallowed and a /// given `MaxDepth`. diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h index aeaf75b..09eb8b9 100644 --- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h +++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h @@ -45,7 +45,7 @@ struct UncheckedOptionalAccessModelOptions { class UncheckedOptionalAccessModel : public DataflowAnalysis<UncheckedOptionalAccessModel, NoopLattice> { public: - UncheckedOptionalAccessModel(ASTContext &Ctx); + UncheckedOptionalAccessModel(ASTContext &Ctx, dataflow::Environment &Env); /// Returns a matcher for the optional classes covered by this model. static ast_matchers::DeclarationMatcher optionalClassDecl(); @@ -54,17 +54,6 @@ public: void transfer(const CFGElement &Elt, NoopLattice &L, Environment &Env); - ComparisonResult compare(QualType Type, const Value &Val1, - const Environment &Env1, const Value &Val2, - const Environment &Env2) override; - - bool merge(QualType Type, const Value &Val1, const Environment &Env1, - const Value &Val2, const Environment &Env2, Value &MergedVal, - Environment &MergedEnv) override; - - Value *widen(QualType Type, Value &Prev, const Environment &PrevEnv, - Value &Current, Environment &CurrentEnv) override; - private: CFGMatchSwitch<TransferState<NoopLattice>> TransferMatchSwitch; }; diff --git a/clang/include/clang/Analysis/FlowSensitive/RecordOps.h b/clang/include/clang/Analysis/FlowSensitive/RecordOps.h index f007a94..7b87840 100644 --- a/clang/include/clang/Analysis/FlowSensitive/RecordOps.h +++ b/clang/include/clang/Analysis/FlowSensitive/RecordOps.h @@ -21,10 +21,10 @@ namespace dataflow { /// Copies a record (struct, class, or union) from `Src` to `Dst`. /// -/// This performs a deep copy, i.e. it copies every field and recurses on -/// fields of record type. It also copies properties from the `RecordValue` -/// associated with `Src` to the `RecordValue` associated with `Dst` (if these -/// `RecordValue`s exist). +/// This performs a deep copy, i.e. it copies every field (including synthetic +/// fields) and recurses on fields of record type. It also copies properties +/// from the `RecordValue` associated with `Src` to the `RecordValue` associated +/// with `Dst` (if these `RecordValue`s exist). /// /// If there is a `RecordValue` associated with `Dst` in the environment, this /// function creates a new `RecordValue` and associates it with `Dst`; clients @@ -47,10 +47,11 @@ void copyRecord(RecordStorageLocation &Src, RecordStorageLocation &Dst, /// retrieved from `Env2`. A convenience overload retrieves values for `Loc1` /// and `Loc2` from the same environment. /// -/// This performs a deep comparison, i.e. it compares every field and recurses -/// on fields of record type. Fields of reference type compare equal if they -/// refer to the same storage location. If `RecordValue`s are associated with -/// `Loc1` and `Loc2`, it also compares the properties on those `RecordValue`s. +/// This performs a deep comparison, i.e. it compares every field (including +/// synthetic fields) and recurses on fields of record type. Fields of reference +/// type compare equal if they refer to the same storage location. If +/// `RecordValue`s are associated with `Loc1` and Loc2`, it also compares the +/// properties on those `RecordValue`s. /// /// Note on how to interpret the result: /// - If this returns true, the records are guaranteed to be equal at runtime. diff --git a/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h b/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h index 89d2bbf..8fcc6a4 100644 --- a/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h +++ b/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h @@ -73,7 +73,13 @@ public: /// /// Contains storage locations for all modeled fields of the record (also /// referred to as "children"). The child map is flat, so accessible members of -/// the base class are directly accesible as children of this location. +/// the base class are directly accessible as children of this location. +/// +/// Record storage locations may also contain so-called synthetic fields. These +/// are typically used to model the internal state of a class (e.g. the value +/// stored in a `std::optional`) without having to depend on that class's +/// implementation details. All `RecordStorageLocation`s of a given type should +/// have the same synthetic fields. /// /// The storage location for a field of reference type may be null. This /// typically occurs in one of two situations: @@ -88,12 +94,12 @@ public: class RecordStorageLocation final : public StorageLocation { public: using FieldToLoc = llvm::DenseMap<const ValueDecl *, StorageLocation *>; + using SyntheticFieldMap = llvm::StringMap<StorageLocation *>; - explicit RecordStorageLocation(QualType Type) - : RecordStorageLocation(Type, FieldToLoc()) {} - - RecordStorageLocation(QualType Type, FieldToLoc TheChildren) - : StorageLocation(Kind::Record, Type), Children(std::move(TheChildren)) { + RecordStorageLocation(QualType Type, FieldToLoc TheChildren, + SyntheticFieldMap TheSyntheticFields) + : StorageLocation(Kind::Record, Type), Children(std::move(TheChildren)), + SyntheticFields(std::move(TheSyntheticFields)) { assert(!Type.isNull()); assert(Type->isRecordType()); assert([this] { @@ -133,6 +139,19 @@ public: return It->second; } + /// Returns the storage location for the synthetic field `Name`. + /// The synthetic field must exist. + StorageLocation &getSyntheticField(llvm::StringRef Name) const { + StorageLocation *Loc = SyntheticFields.lookup(Name); + assert(Loc != nullptr); + return *Loc; + } + + llvm::iterator_range<SyntheticFieldMap::const_iterator> + synthetic_fields() const { + return {SyntheticFields.begin(), SyntheticFields.end()}; + } + /// Changes the child storage location for a field `D` of reference type. /// All other fields cannot change their storage location and always retain /// the storage location passed to the `RecordStorageLocation` constructor. @@ -151,6 +170,7 @@ public: private: FieldToLoc Children; + SyntheticFieldMap SyntheticFields; }; } // namespace dataflow diff --git a/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp index 0a2fcd4..fa11497 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp @@ -68,11 +68,38 @@ StorageLocation &DataflowAnalysisContext::createStorageLocation(QualType Type) { else FieldLocs.insert({Field, &createStorageLocation( Field->getType().getNonReferenceType())}); - return arena().create<RecordStorageLocation>(Type, std::move(FieldLocs)); + + RecordStorageLocation::SyntheticFieldMap SyntheticFields; + for (const auto &Entry : getSyntheticFields(Type)) + SyntheticFields.insert( + {Entry.getKey(), + &createStorageLocation(Entry.getValue().getNonReferenceType())}); + + return createRecordStorageLocation(Type, std::move(FieldLocs), + std::move(SyntheticFields)); } return arena().create<ScalarStorageLocation>(Type); } +// Returns the keys for a given `StringMap`. +// Can't use `StringSet` as the return type as it doesn't support `operator==`. +template <typename T> +static llvm::DenseSet<llvm::StringRef> getKeys(const llvm::StringMap<T> &Map) { + return llvm::DenseSet<llvm::StringRef>(Map.keys().begin(), Map.keys().end()); +} + +RecordStorageLocation &DataflowAnalysisContext::createRecordStorageLocation( + QualType Type, RecordStorageLocation::FieldToLoc FieldLocs, + RecordStorageLocation::SyntheticFieldMap SyntheticFields) { + assert(Type->isRecordType()); + assert(containsSameFields(getModeledFields(Type), FieldLocs)); + assert(getKeys(getSyntheticFields(Type)) == getKeys(SyntheticFields)); + + RecordStorageLocationCreated = true; + return arena().create<RecordStorageLocation>(Type, std::move(FieldLocs), + std::move(SyntheticFields)); +} + StorageLocation & DataflowAnalysisContext::getStableStorageLocation(const ValueDecl &D) { if (auto *Loc = DeclToLoc.lookup(&D)) @@ -367,3 +394,14 @@ clang::dataflow::FieldSet clang::dataflow::getObjectFields(QualType Type) { getFieldsFromClassHierarchy(Type, Fields); return Fields; } + +bool clang::dataflow::containsSameFields( + const clang::dataflow::FieldSet &Fields, + const clang::dataflow::RecordStorageLocation::FieldToLoc &FieldLocs) { + if (Fields.size() != FieldLocs.size()) + return false; + for ([[maybe_unused]] auto [Field, Loc] : FieldLocs) + if (!Fields.contains(cast_or_null<FieldDecl>(Field))) + return false; + return true; +} diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp index 525ab18..042402a 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -367,6 +367,59 @@ getFieldsGlobalsAndFuncs(const Stmt &S, FieldSet &Fields, } } +Environment::Environment(DataflowAnalysisContext &DACtx) + : DACtx(&DACtx), + FlowConditionToken(DACtx.arena().makeFlowConditionToken()) {} + +Environment::Environment(DataflowAnalysisContext &DACtx, + const DeclContext &DeclCtx) + : Environment(DACtx) { + CallStack.push_back(&DeclCtx); +} + +void Environment::initialize() { + const DeclContext *DeclCtx = getDeclCtx(); + if (DeclCtx == nullptr) + return; + + if (const auto *FuncDecl = dyn_cast<FunctionDecl>(DeclCtx)) { + assert(FuncDecl->getBody() != nullptr); + + initFieldsGlobalsAndFuncs(FuncDecl); + + for (const auto *ParamDecl : FuncDecl->parameters()) { + assert(ParamDecl != nullptr); + setStorageLocation(*ParamDecl, createObject(*ParamDecl, nullptr)); + } + } + + if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(DeclCtx)) { + auto *Parent = MethodDecl->getParent(); + assert(Parent != nullptr); + + if (Parent->isLambda()) { + for (auto Capture : Parent->captures()) { + if (Capture.capturesVariable()) { + const auto *VarDecl = Capture.getCapturedVar(); + assert(VarDecl != nullptr); + setStorageLocation(*VarDecl, createObject(*VarDecl, nullptr)); + } else if (Capture.capturesThis()) { + const auto *SurroundingMethodDecl = + cast<CXXMethodDecl>(DeclCtx->getNonClosureAncestor()); + QualType ThisPointeeType = + SurroundingMethodDecl->getFunctionObjectParameterType(); + setThisPointeeStorageLocation( + cast<RecordValue>(createValue(ThisPointeeType))->getLoc()); + } + } + } else if (MethodDecl->isImplicitObjectMemberFunction()) { + QualType ThisPointeeType = MethodDecl->getFunctionObjectParameterType(); + setThisPointeeStorageLocation( + cast<RecordValue>(createValue(ThisPointeeType))->getLoc()); + } + } +} + // FIXME: Add support for resetting globals after function calls to enable // the implementation of sound analyses. void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) { @@ -416,59 +469,12 @@ void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) { } } -Environment::Environment(DataflowAnalysisContext &DACtx) - : DACtx(&DACtx), - FlowConditionToken(DACtx.arena().makeFlowConditionToken()) {} - Environment Environment::fork() const { Environment Copy(*this); Copy.FlowConditionToken = DACtx->forkFlowCondition(FlowConditionToken); return Copy; } -Environment::Environment(DataflowAnalysisContext &DACtx, - const DeclContext &DeclCtx) - : Environment(DACtx) { - CallStack.push_back(&DeclCtx); - - if (const auto *FuncDecl = dyn_cast<FunctionDecl>(&DeclCtx)) { - assert(FuncDecl->getBody() != nullptr); - - initFieldsGlobalsAndFuncs(FuncDecl); - - for (const auto *ParamDecl : FuncDecl->parameters()) { - assert(ParamDecl != nullptr); - setStorageLocation(*ParamDecl, createObject(*ParamDecl, nullptr)); - } - } - - if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(&DeclCtx)) { - auto *Parent = MethodDecl->getParent(); - assert(Parent != nullptr); - - if (Parent->isLambda()) { - for (auto Capture : Parent->captures()) { - if (Capture.capturesVariable()) { - const auto *VarDecl = Capture.getCapturedVar(); - assert(VarDecl != nullptr); - setStorageLocation(*VarDecl, createObject(*VarDecl, nullptr)); - } else if (Capture.capturesThis()) { - const auto *SurroundingMethodDecl = - cast<CXXMethodDecl>(DeclCtx.getNonClosureAncestor()); - QualType ThisPointeeType = - SurroundingMethodDecl->getFunctionObjectParameterType(); - ThisPointeeLoc = - &cast<RecordValue>(createValue(ThisPointeeType))->getLoc(); - } - } - } else if (MethodDecl->isImplicitObjectMemberFunction()) { - QualType ThisPointeeType = MethodDecl->getFunctionObjectParameterType(); - ThisPointeeLoc = - &cast<RecordValue>(createValue(ThisPointeeType))->getLoc(); - } - } -} - bool Environment::canDescend(unsigned MaxDepth, const DeclContext *Callee) const { return CallStack.size() <= MaxDepth && !llvm::is_contained(CallStack, Callee); @@ -727,10 +733,6 @@ StorageLocation *Environment::getStorageLocation(const Expr &E) const { return getStorageLocationInternal(E); } -RecordStorageLocation *Environment::getThisPointeeStorageLocation() const { - return ThisPointeeLoc; -} - RecordStorageLocation & Environment::getResultObjectLocation(const Expr &RecordPRValue) { assert(RecordPRValue.getType()->isRecordType()); @@ -852,8 +854,16 @@ Value *Environment::createValueUnlessSelfReferential( CreatedValuesCount)}); } - RecordStorageLocation &Loc = - arena().create<RecordStorageLocation>(Type, std::move(FieldLocs)); + RecordStorageLocation::SyntheticFieldMap SyntheticFieldLocs; + for (const auto &Entry : DACtx->getSyntheticFields(Type)) { + SyntheticFieldLocs.insert( + {Entry.getKey(), + &createLocAndMaybeValue(Entry.getValue(), Visited, Depth + 1, + CreatedValuesCount)}); + } + + RecordStorageLocation &Loc = DACtx->createRecordStorageLocation( + Type, std::move(FieldLocs), std::move(SyntheticFieldLocs)); RecordValue &RecordVal = create<RecordValue>(Loc); // As we already have a storage location for the `RecordValue`, we can and diff --git a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp index 8329367..7430ef5 100644 --- a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp +++ b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp @@ -136,6 +136,10 @@ public: if (Value *Val = Env.getValue(*Child.second)) dump(*Val); }); + + for (const auto &SyntheticField : RLoc->synthetic_fields()) + JOS.attributeObject(("sf:" + SyntheticField.first()).str(), + [&] { dump(*SyntheticField.second); }); } } diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 55d0713..69ac2c2 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -122,12 +122,6 @@ auto nulloptTypeDecl() { auto hasNulloptType() { return hasType(nulloptTypeDecl()); } -// `optional` or `nullopt_t` -auto hasAnyOptionalType() { - return hasType(hasUnqualifiedDesugaredType( - recordType(hasDeclaration(anyOf(nulloptTypeDecl(), optionalClass()))))); -} - auto inPlaceClass() { return recordDecl(hasAnyName("std::in_place_t", "absl::in_place_t", "base::in_place_t", "folly::in_place_t")); @@ -162,11 +156,6 @@ auto isOptionalValueOrConversionAssignment() { argumentCountIs(2), hasArgument(1, unless(hasNulloptType()))); } -auto isNulloptConstructor() { - return cxxConstructExpr(hasNulloptType(), argumentCountIs(1), - hasArgument(0, hasNulloptType())); -} - auto isOptionalNulloptAssignment() { return cxxOperatorCallExpr(hasOverloadedOperatorName("="), callee(cxxMethodDecl(ofClass(optionalClass()))), @@ -246,10 +235,19 @@ const Formula &forceBoolValue(Environment &Env, const Expr &Expr) { return Value->formula(); } +StorageLocation &locForHasValue(const RecordStorageLocation &OptionalLoc) { + return OptionalLoc.getSyntheticField("has_value"); +} + +StorageLocation &locForValue(const RecordStorageLocation &OptionalLoc) { + return OptionalLoc.getSyntheticField("value"); +} + /// Sets `HasValueVal` as the symbolic value that represents the "has_value" -/// property of the optional value `OptionalVal`. -void setHasValue(Value &OptionalVal, BoolValue &HasValueVal) { - OptionalVal.setProperty("has_value", HasValueVal); +/// property of the optional at `OptionalLoc`. +void setHasValue(RecordStorageLocation &OptionalLoc, BoolValue &HasValueVal, + Environment &Env) { + Env.setValue(locForHasValue(OptionalLoc), HasValueVal); } /// Creates a symbolic value for an `optional` value at an existing storage @@ -259,23 +257,22 @@ RecordValue &createOptionalValue(RecordStorageLocation &Loc, BoolValue &HasValueVal, Environment &Env) { auto &OptionalVal = Env.create<RecordValue>(Loc); Env.setValue(Loc, OptionalVal); - setHasValue(OptionalVal, HasValueVal); + setHasValue(Loc, HasValueVal, Env); return OptionalVal; } /// Returns the symbolic value that represents the "has_value" property of the -/// optional value `OptionalVal`. Returns null if `OptionalVal` is null. -BoolValue *getHasValue(Environment &Env, Value *OptionalVal) { - if (OptionalVal != nullptr) { - auto *HasValueVal = - cast_or_null<BoolValue>(OptionalVal->getProperty("has_value")); - if (HasValueVal == nullptr) { - HasValueVal = &Env.makeAtomicBoolValue(); - OptionalVal->setProperty("has_value", *HasValueVal); - } - return HasValueVal; +/// optional at `OptionalLoc`. Returns null if `OptionalLoc` is null. +BoolValue *getHasValue(Environment &Env, RecordStorageLocation *OptionalLoc) { + if (OptionalLoc == nullptr) + return nullptr; + StorageLocation &HasValueLoc = locForHasValue(*OptionalLoc); + auto *HasValueVal = cast_or_null<BoolValue>(Env.getValue(HasValueLoc)); + if (HasValueVal == nullptr) { + HasValueVal = &Env.makeAtomicBoolValue(); + Env.setValue(HasValueLoc, *HasValueVal); } - return nullptr; + return HasValueVal; } /// Returns true if and only if `Type` is an optional type. @@ -302,155 +299,31 @@ int countOptionalWrappers(const ASTContext &ASTCtx, QualType Type) { .getDesugaredType(ASTCtx)); } -/// Tries to initialize the `optional`'s value (that is, contents), and return -/// its location. Returns nullptr if the value can't be represented. -StorageLocation *maybeInitializeOptionalValueMember(QualType Q, - Value &OptionalVal, - Environment &Env) { - // The "value" property represents a synthetic field. As such, it needs - // `StorageLocation`, like normal fields (and other variables). So, we model - // it with a `PointerValue`, since that includes a storage location. Once - // the property is set, it will be shared by all environments that access the - // `Value` representing the optional (here, `OptionalVal`). - if (auto *ValueProp = OptionalVal.getProperty("value")) { - auto *ValuePtr = clang::cast<PointerValue>(ValueProp); - auto &ValueLoc = ValuePtr->getPointeeLoc(); - if (Env.getValue(ValueLoc) != nullptr) - return &ValueLoc; - - // The property was previously set, but the value has been lost. This can - // happen in various situations, for example: - // - Because of an environment merge (where the two environments mapped the - // property to different values, which resulted in them both being - // discarded). - // - When two blocks in the CFG, with neither a dominator of the other, - // visit the same optional value. (FIXME: This is something we can and - // should fix -- see also the lengthy FIXME below.) - // - Or even when a block is revisited during testing to collect - // per-statement state. - // FIXME: This situation means that the optional contents are not shared - // between branches and the like. Practically, this lack of sharing - // reduces the precision of the model when the contents are relevant to - // the check, like another optional or a boolean that influences control - // flow. - if (ValueLoc.getType()->isRecordType()) { - refreshRecordValue(cast<RecordStorageLocation>(ValueLoc), Env); - return &ValueLoc; - } else { - auto *ValueVal = Env.createValue(ValueLoc.getType()); - if (ValueVal == nullptr) - return nullptr; - Env.setValue(ValueLoc, *ValueVal); - return &ValueLoc; - } - } - - auto Ty = Q.getNonReferenceType(); - auto &ValueLoc = Env.createObject(Ty); - auto &ValuePtr = Env.create<PointerValue>(ValueLoc); - // FIXME: - // The change we make to the `value` property below may become visible to - // other blocks that aren't successors of the current block and therefore - // don't see the change we made above mapping `ValueLoc` to `ValueVal`. For - // example: - // - // void target(optional<int> oo, bool b) { - // // `oo` is associated with a `RecordValue` here, which we will call - // // `OptionalVal`. - // - // // The `has_value` property is set on `OptionalVal` (but not the - // // `value` property yet). - // if (!oo.has_value()) return; - // - // if (b) { - // // Let's assume we transfer the `if` branch first. - // // - // // This causes us to call `maybeInitializeOptionalValueMember()`, - // // which causes us to set the `value` property on `OptionalVal` - // // (which had not been set until this point). This `value` property - // // refers to a `PointerValue`, which in turn refers to a - // // StorageLocation` that is associated to an `IntegerValue`. - // oo.value(); - // } else { - // // Let's assume we transfer the `else` branch after the `if` branch. - // // - // // We see the `value` property that the `if` branch set on - // // `OptionalVal`, but in the environment for this block, the - // // `StorageLocation` in the `PointerValue` is not associated with any - // // `Value`. - // oo.value(); - // } - // } - // - // This situation is currently "saved" by the code above that checks whether - // the `value` property is already set, and if, the `ValueLoc` is not - // associated with a `ValueVal`, creates a new `ValueVal`. - // - // However, what we should really do is to make sure that the change to the - // `value` property does not "leak" to other blocks that are not successors - // of this block. To do this, instead of simply setting the `value` property - // on the existing `OptionalVal`, we should create a new `Value` for the - // optional, set the property on that, and associate the storage location that - // is currently associated with the existing `OptionalVal` with the newly - // created `Value` instead. - OptionalVal.setProperty("value", ValuePtr); - return &ValueLoc; -} - -void initializeOptionalReference(const Expr *OptionalExpr, - const MatchFinder::MatchResult &, - LatticeTransferState &State) { - if (auto *OptionalVal = State.Env.getValue(*OptionalExpr)) { - if (OptionalVal->getProperty("has_value") == nullptr) { - setHasValue(*OptionalVal, State.Env.makeAtomicBoolValue()); - } +StorageLocation *getLocBehindPossiblePointer(const Expr &E, + const Environment &Env) { + if (E.isPRValue()) { + if (auto *PointerVal = dyn_cast_or_null<PointerValue>(Env.getValue(E))) + return &PointerVal->getPointeeLoc(); + return nullptr; } -} - -/// Returns true if and only if `OptionalVal` is initialized and known to be -/// empty in `Env`. -bool isEmptyOptional(const Value &OptionalVal, const Environment &Env) { - auto *HasValueVal = - cast_or_null<BoolValue>(OptionalVal.getProperty("has_value")); - return HasValueVal != nullptr && - Env.proves(Env.arena().makeNot(HasValueVal->formula())); -} - -/// Returns true if and only if `OptionalVal` is initialized and known to be -/// non-empty in `Env`. -bool isNonEmptyOptional(const Value &OptionalVal, const Environment &Env) { - auto *HasValueVal = - cast_or_null<BoolValue>(OptionalVal.getProperty("has_value")); - return HasValueVal != nullptr && Env.proves(HasValueVal->formula()); -} - -Value *getValueBehindPossiblePointer(const Expr &E, const Environment &Env) { - Value *Val = Env.getValue(E); - if (auto *PointerVal = dyn_cast_or_null<PointerValue>(Val)) - return Env.getValue(PointerVal->getPointeeLoc()); - return Val; + return Env.getStorageLocation(E); } void transferUnwrapCall(const Expr *UnwrapExpr, const Expr *ObjectExpr, LatticeTransferState &State) { - if (auto *OptionalVal = - getValueBehindPossiblePointer(*ObjectExpr, State.Env)) { + if (auto *OptionalLoc = cast_or_null<RecordStorageLocation>( + getLocBehindPossiblePointer(*ObjectExpr, State.Env))) { if (State.Env.getStorageLocation(*UnwrapExpr) == nullptr) - if (auto *Loc = maybeInitializeOptionalValueMember( - UnwrapExpr->getType(), *OptionalVal, State.Env)) - State.Env.setStorageLocation(*UnwrapExpr, *Loc); + State.Env.setStorageLocation(*UnwrapExpr, locForValue(*OptionalLoc)); } } void transferArrowOpCall(const Expr *UnwrapExpr, const Expr *ObjectExpr, LatticeTransferState &State) { - if (auto *OptionalVal = - getValueBehindPossiblePointer(*ObjectExpr, State.Env)) { - if (auto *Loc = maybeInitializeOptionalValueMember( - UnwrapExpr->getType()->getPointeeType(), *OptionalVal, State.Env)) { - State.Env.setValue(*UnwrapExpr, State.Env.create<PointerValue>(*Loc)); - } - } + if (auto *OptionalLoc = cast_or_null<RecordStorageLocation>( + getLocBehindPossiblePointer(*ObjectExpr, State.Env))) + State.Env.setValue( + *UnwrapExpr, State.Env.create<PointerValue>(locForValue(*OptionalLoc))); } void transferMakeOptionalCall(const CallExpr *E, @@ -465,8 +338,7 @@ void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr, const MatchFinder::MatchResult &, LatticeTransferState &State) { if (auto *HasValueVal = getHasValue( - State.Env, getValueBehindPossiblePointer( - *CallExpr->getImplicitObjectArgument(), State.Env))) { + State.Env, getImplicitObjectLocation(*CallExpr, State.Env))) { State.Env.setValue(*CallExpr, *HasValueVal); } } @@ -480,12 +352,11 @@ void transferValueOrImpl( const Formula &HasValueVal)) { auto &Env = State.Env; - const auto *ObjectArgumentExpr = - Result.Nodes.getNodeAs<clang::CXXMemberCallExpr>(ValueOrCallID) - ->getImplicitObjectArgument(); + const auto *MCE = + Result.Nodes.getNodeAs<clang::CXXMemberCallExpr>(ValueOrCallID); - auto *HasValueVal = getHasValue( - State.Env, getValueBehindPossiblePointer(*ObjectArgumentExpr, State.Env)); + auto *HasValueVal = + getHasValue(State.Env, getImplicitObjectLocation(*MCE, State.Env)); if (HasValueVal == nullptr) return; @@ -578,7 +449,9 @@ BoolValue &valueOrConversionHasValue(const FunctionDecl &F, const Expr &E, // This is a constructor/assignment call for `optional<T>` with argument of // type `optional<U>` such that `T` is constructible from `U`. - if (auto *HasValueVal = getHasValue(State.Env, State.Env.getValue(E))) + auto *Loc = + cast_or_null<RecordStorageLocation>(State.Env.getStorageLocation(E)); + if (auto *HasValueVal = getHasValue(State.Env, Loc)) return *HasValueVal; return State.Env.makeAtomicBoolValue(); } @@ -645,11 +518,11 @@ void transferSwap(RecordStorageLocation *Loc1, RecordStorageLocation *Loc2, // allows for local reasoning about the value. To avoid the above, we would // need *lazy* value allocation. // FIXME: allocate values lazily, instead of just creating a fresh value. - BoolValue *BoolVal1 = getHasValue(Env, Env.getValue(*Loc1)); + BoolValue *BoolVal1 = getHasValue(Env, Loc1); if (BoolVal1 == nullptr) BoolVal1 = &Env.makeAtomicBoolValue(); - BoolValue *BoolVal2 = getHasValue(Env, Env.getValue(*Loc2)); + BoolValue *BoolVal2 = getHasValue(Env, Loc2); if (BoolVal2 == nullptr) BoolVal2 = &Env.makeAtomicBoolValue(); @@ -712,20 +585,26 @@ void transferOptionalAndOptionalCmp(const clang::CXXOperatorCallExpr *CmpExpr, Environment &Env = State.Env; auto &A = Env.arena(); auto *CmpValue = &forceBoolValue(Env, *CmpExpr); - if (auto *LHasVal = getHasValue(Env, Env.getValue(*CmpExpr->getArg(0)))) - if (auto *RHasVal = getHasValue(Env, Env.getValue(*CmpExpr->getArg(1)))) { + auto *Arg0Loc = cast_or_null<RecordStorageLocation>( + Env.getStorageLocation(*CmpExpr->getArg(0))); + if (auto *LHasVal = getHasValue(Env, Arg0Loc)) { + auto *Arg1Loc = cast_or_null<RecordStorageLocation>( + Env.getStorageLocation(*CmpExpr->getArg(1))); + if (auto *RHasVal = getHasValue(Env, Arg1Loc)) { if (CmpExpr->getOperator() == clang::OO_ExclaimEqual) CmpValue = &A.makeNot(*CmpValue); Env.assume(evaluateEquality(A, *CmpValue, LHasVal->formula(), RHasVal->formula())); } + } } void transferOptionalAndValueCmp(const clang::CXXOperatorCallExpr *CmpExpr, const clang::Expr *E, Environment &Env) { auto &A = Env.arena(); auto *CmpValue = &forceBoolValue(Env, *CmpExpr); - if (auto *HasVal = getHasValue(Env, Env.getValue(*E))) { + auto *Loc = cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*E)); + if (auto *HasVal = getHasValue(Env, Loc)) { if (CmpExpr->getOperator() == clang::OO_ExclaimEqual) CmpValue = &A.makeNot(*CmpValue); Env.assume( @@ -733,6 +612,19 @@ void transferOptionalAndValueCmp(const clang::CXXOperatorCallExpr *CmpExpr, } } +void transferOptionalAndNulloptCmp(const clang::CXXOperatorCallExpr *CmpExpr, + const clang::Expr *E, Environment &Env) { + auto &A = Env.arena(); + auto *CmpValue = &forceBoolValue(Env, *CmpExpr); + auto *Loc = cast_or_null<RecordStorageLocation>(Env.getStorageLocation(*E)); + if (auto *HasVal = getHasValue(Env, Loc)) { + if (CmpExpr->getOperator() == clang::OO_ExclaimEqual) + CmpValue = &A.makeNot(*CmpValue); + Env.assume(evaluateEquality(A, *CmpValue, HasVal->formula(), + A.makeLiteral(false))); + } +} + std::optional<StatementMatcher> ignorableOptional(const UncheckedOptionalAccessModelOptions &Options) { if (Options.IgnoreSmartPointerDereference) { @@ -762,12 +654,6 @@ auto buildTransferMatchSwitch() { // lot of duplicated work (e.g. string comparisons), consider providing APIs // that avoid it through memoization. return CFGMatchSwitchBuilder<LatticeTransferState>() - // Attach a symbolic "has_value" state to optional values that we see for - // the first time. - .CaseOfCFGStmt<Expr>( - expr(anyOf(declRefExpr(), memberExpr()), hasOptionalType()), - initializeOptionalReference) - // make_optional .CaseOfCFGStmt<CallExpr>(isMakeOptionalCall(), transferMakeOptionalCall) @@ -779,14 +665,6 @@ auto buildTransferMatchSwitch() { constructOptionalValue(*E, State.Env, State.Env.getBoolLiteralValue(true)); }) - // nullopt_t::nullopt_t - .CaseOfCFGStmt<CXXConstructExpr>( - isNulloptConstructor(), - [](const CXXConstructExpr *E, const MatchFinder::MatchResult &, - LatticeTransferState &State) { - constructOptionalValue(*E, State.Env, - State.Env.getBoolLiteralValue(false)); - }) // optional::optional(nullopt_t) .CaseOfCFGStmt<CXXConstructExpr>( isOptionalNulloptConstructor(), @@ -887,18 +765,32 @@ auto buildTransferMatchSwitch() { // Comparisons (==, !=): .CaseOfCFGStmt<CXXOperatorCallExpr>( - isComparisonOperatorCall(hasAnyOptionalType(), hasAnyOptionalType()), + isComparisonOperatorCall(hasOptionalType(), hasOptionalType()), transferOptionalAndOptionalCmp) .CaseOfCFGStmt<CXXOperatorCallExpr>( - isComparisonOperatorCall(hasOptionalType(), - unless(hasAnyOptionalType())), + isComparisonOperatorCall(hasOptionalType(), hasNulloptType()), + [](const clang::CXXOperatorCallExpr *Cmp, + const MatchFinder::MatchResult &, LatticeTransferState &State) { + transferOptionalAndNulloptCmp(Cmp, Cmp->getArg(0), State.Env); + }) + .CaseOfCFGStmt<CXXOperatorCallExpr>( + isComparisonOperatorCall(hasNulloptType(), hasOptionalType()), + [](const clang::CXXOperatorCallExpr *Cmp, + const MatchFinder::MatchResult &, LatticeTransferState &State) { + transferOptionalAndNulloptCmp(Cmp, Cmp->getArg(1), State.Env); + }) + .CaseOfCFGStmt<CXXOperatorCallExpr>( + isComparisonOperatorCall( + hasOptionalType(), + unless(anyOf(hasOptionalType(), hasNulloptType()))), [](const clang::CXXOperatorCallExpr *Cmp, const MatchFinder::MatchResult &, LatticeTransferState &State) { transferOptionalAndValueCmp(Cmp, Cmp->getArg(0), State.Env); }) .CaseOfCFGStmt<CXXOperatorCallExpr>( - isComparisonOperatorCall(unless(hasAnyOptionalType()), - hasOptionalType()), + isComparisonOperatorCall( + unless(anyOf(hasOptionalType(), hasNulloptType())), + hasOptionalType()), [](const clang::CXXOperatorCallExpr *Cmp, const MatchFinder::MatchResult &, LatticeTransferState &State) { transferOptionalAndValueCmp(Cmp, Cmp->getArg(1), State.Env); @@ -913,8 +805,9 @@ auto buildTransferMatchSwitch() { llvm::SmallVector<SourceLocation> diagnoseUnwrapCall(const Expr *ObjectExpr, const Environment &Env) { - if (auto *OptionalVal = getValueBehindPossiblePointer(*ObjectExpr, Env)) { - auto *Prop = OptionalVal->getProperty("has_value"); + if (auto *OptionalLoc = cast_or_null<RecordStorageLocation>( + getLocBehindPossiblePointer(*ObjectExpr, Env))) { + auto *Prop = Env.getValue(locForHasValue(*OptionalLoc)); if (auto *HasValueVal = cast_or_null<BoolValue>(Prop)) { if (Env.proves(HasValueVal->formula())) return {}; @@ -960,9 +853,24 @@ UncheckedOptionalAccessModel::optionalClassDecl() { return optionalClass(); } -UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx) +static QualType valueTypeFromOptionalType(QualType OptionalTy) { + auto *CTSD = + cast<ClassTemplateSpecializationDecl>(OptionalTy->getAsCXXRecordDecl()); + return CTSD->getTemplateArgs()[0].getAsType(); +} + +UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx, + Environment &Env) : DataflowAnalysis<UncheckedOptionalAccessModel, NoopLattice>(Ctx), - TransferMatchSwitch(buildTransferMatchSwitch()) {} + TransferMatchSwitch(buildTransferMatchSwitch()) { + Env.getDataflowAnalysisContext().setSyntheticFieldCallback( + [&Ctx](QualType Ty) -> llvm::StringMap<QualType> { + if (!isOptionalType(Ty)) + return {}; + return {{"value", valueTypeFromOptionalType(Ty)}, + {"has_value", Ctx.BoolTy}}; + }); +} void UncheckedOptionalAccessModel::transfer(const CFGElement &Elt, NoopLattice &L, Environment &Env) { @@ -970,76 +878,6 @@ void UncheckedOptionalAccessModel::transfer(const CFGElement &Elt, TransferMatchSwitch(Elt, getASTContext(), State); } -ComparisonResult UncheckedOptionalAccessModel::compare( - QualType Type, const Value &Val1, const Environment &Env1, - const Value &Val2, const Environment &Env2) { - if (!isOptionalType(Type)) - return ComparisonResult::Unknown; - bool MustNonEmpty1 = isNonEmptyOptional(Val1, Env1); - bool MustNonEmpty2 = isNonEmptyOptional(Val2, Env2); - if (MustNonEmpty1 && MustNonEmpty2) - return ComparisonResult::Same; - // If exactly one is true, then they're different, no reason to check whether - // they're definitely empty. - if (MustNonEmpty1 || MustNonEmpty2) - return ComparisonResult::Different; - // Check if they're both definitely empty. - return (isEmptyOptional(Val1, Env1) && isEmptyOptional(Val2, Env2)) - ? ComparisonResult::Same - : ComparisonResult::Different; -} - -bool UncheckedOptionalAccessModel::merge(QualType Type, const Value &Val1, - const Environment &Env1, - const Value &Val2, - const Environment &Env2, - Value &MergedVal, - Environment &MergedEnv) { - if (!isOptionalType(Type)) - return true; - // FIXME: uses same approach as join for `BoolValues`. Requires non-const - // values, though, so will require updating the interface. - auto &HasValueVal = MergedEnv.makeAtomicBoolValue(); - bool MustNonEmpty1 = isNonEmptyOptional(Val1, Env1); - bool MustNonEmpty2 = isNonEmptyOptional(Val2, Env2); - if (MustNonEmpty1 && MustNonEmpty2) - MergedEnv.assume(HasValueVal.formula()); - else if ( - // Only make the costly calls to `isEmptyOptional` if we got "unknown" - // (false) for both calls to `isNonEmptyOptional`. - !MustNonEmpty1 && !MustNonEmpty2 && isEmptyOptional(Val1, Env1) && - isEmptyOptional(Val2, Env2)) - MergedEnv.assume(MergedEnv.arena().makeNot(HasValueVal.formula())); - setHasValue(MergedVal, HasValueVal); - return true; -} - -Value *UncheckedOptionalAccessModel::widen(QualType Type, Value &Prev, - const Environment &PrevEnv, - Value &Current, - Environment &CurrentEnv) { - switch (compare(Type, Prev, PrevEnv, Current, CurrentEnv)) { - case ComparisonResult::Same: - return &Prev; - case ComparisonResult::Different: - if (auto *PrevHasVal = - cast_or_null<BoolValue>(Prev.getProperty("has_value"))) { - if (isa<TopBoolValue>(PrevHasVal)) - return &Prev; - } - if (auto *CurrentHasVal = - cast_or_null<BoolValue>(Current.getProperty("has_value"))) { - if (isa<TopBoolValue>(CurrentHasVal)) - return &Current; - } - return &createOptionalValue(cast<RecordValue>(Current).getLoc(), - CurrentEnv.makeTopBoolValue(), CurrentEnv); - case ComparisonResult::Unknown: - return nullptr; - } - llvm_unreachable("all cases covered in switch"); -} - UncheckedOptionalAccessDiagnoser::UncheckedOptionalAccessDiagnoser( UncheckedOptionalAccessModelOptions Options) : DiagnoseMatchSwitch(buildDiagnoseMatchSwitch(Options)) {} diff --git a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp index 38638f8..caaf443 100644 --- a/clang/lib/Analysis/FlowSensitive/RecordOps.cpp +++ b/clang/lib/Analysis/FlowSensitive/RecordOps.cpp @@ -54,6 +54,18 @@ void clang::dataflow::copyRecord(RecordStorageLocation &Src, } } + for (const auto &[Name, SynthFieldLoc] : Src.synthetic_fields()) { + if (SynthFieldLoc->getType()->isRecordType()) { + copyRecord(*cast<RecordStorageLocation>(SynthFieldLoc), + cast<RecordStorageLocation>(Dst.getSyntheticField(Name)), Env); + } else { + if (Value *Val = Env.getValue(*SynthFieldLoc)) + Env.setValue(Dst.getSyntheticField(Name), *Val); + else + Env.clearValue(Dst.getSyntheticField(Name)); + } + } + RecordValue *SrcVal = cast_or_null<RecordValue>(Env.getValue(Src)); RecordValue *DstVal = cast_or_null<RecordValue>(Env.getValue(Dst)); @@ -101,6 +113,18 @@ bool clang::dataflow::recordsEqual(const RecordStorageLocation &Loc1, } } + for (const auto &[Name, SynthFieldLoc1] : Loc1.synthetic_fields()) { + if (SynthFieldLoc1->getType()->isRecordType()) { + if (!recordsEqual( + *cast<RecordStorageLocation>(SynthFieldLoc1), Env1, + cast<RecordStorageLocation>(Loc2.getSyntheticField(Name)), Env2)) + return false; + } else if (Env1.getValue(*SynthFieldLoc1) != + Env2.getValue(Loc2.getSyntheticField(Name))) { + return false; + } + } + llvm::StringMap<Value *> Props1, Props2; if (RecordValue *Val1 = cast_or_null<RecordValue>(Env1.getValue(Loc1))) diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp index 4343af79..bbf5f12 100644 --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -703,20 +703,18 @@ public: // `InitListExpr`, all fields in the class, including those from base // classes, are included in the set of modeled fields. The code above // should therefore populate exactly the modeled fields. - assert([&]() { - auto ModeledFields = - Env.getDataflowAnalysisContext().getModeledFields(Type); - if (ModeledFields.size() != FieldLocs.size()) - return false; - for ([[maybe_unused]] auto [Field, Loc] : FieldLocs) - if (!ModeledFields.contains(cast_or_null<FieldDecl>(Field))) - return false; - return true; - }()); - - auto &Loc = - Env.getDataflowAnalysisContext().arena().create<RecordStorageLocation>( - Type, std::move(FieldLocs)); + assert(containsSameFields( + Env.getDataflowAnalysisContext().getModeledFields(Type), FieldLocs)); + + RecordStorageLocation::SyntheticFieldMap SyntheticFieldLocs; + for (const auto &Entry : + Env.getDataflowAnalysisContext().getSyntheticFields(Type)) { + SyntheticFieldLocs.insert( + {Entry.getKey(), &Env.createObject(Entry.getValue())}); + } + + auto &Loc = Env.getDataflowAnalysisContext().createRecordStorageLocation( + Type, std::move(FieldLocs), std::move(SyntheticFieldLocs)); RecordValue &RecordVal = Env.create<RecordValue>(Loc); Env.setValue(Loc, RecordVal); diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp index ade8c84..8c93602 100644 --- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp +++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp @@ -501,6 +501,14 @@ runTypeErasedDataflowAnalysis( PostVisitCFG) { PrettyStackTraceAnalysis CrashInfo(CFCtx, "runTypeErasedDataflowAnalysis"); + std::optional<Environment> MaybeStartingEnv; + if (InitEnv.callStackSize() == 1) { + MaybeStartingEnv = InitEnv.fork(); + MaybeStartingEnv->initialize(); + } + const Environment &StartingEnv = + MaybeStartingEnv ? *MaybeStartingEnv : InitEnv; + const clang::CFG &CFG = CFCtx.getCFG(); PostOrderCFGView POV(&CFG); ForwardDataflowWorklist Worklist(CFG, &POV); @@ -511,10 +519,10 @@ runTypeErasedDataflowAnalysis( // The entry basic block doesn't contain statements so it can be skipped. const CFGBlock &Entry = CFG.getEntry(); BlockStates[Entry.getBlockID()] = {Analysis.typeErasedInitialElement(), - InitEnv.fork()}; + StartingEnv.fork()}; Worklist.enqueueSuccessors(&Entry); - AnalysisContext AC(CFCtx, Analysis, InitEnv, BlockStates); + AnalysisContext AC(CFCtx, Analysis, StartingEnv, BlockStates); // Bugs in lattices and transfer functions can prevent the analysis from // converging. To limit the damage (infinite loops) that these bugs can cause, diff --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp index ff6cbec..3569b0e 100644 --- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp @@ -96,6 +96,7 @@ TEST_F(EnvironmentTest, CreateValueRecursiveType) { // Verify that the struct and the field (`R`) with first appearance of the // type is created successfully. Environment Env(DAContext, *Fun); + Env.initialize(); auto &SLoc = cast<RecordStorageLocation>(Env.createObject(Ty)); PointerValue *PV = cast_or_null<PointerValue>(getFieldValue(&SLoc, *R, Env)); EXPECT_THAT(PV, NotNull()); @@ -175,6 +176,7 @@ TEST_F(EnvironmentTest, InitGlobalVarsFun) { // Verify the global variable is populated when we analyze `Target`. Environment Env(DAContext, *Fun); + Env.initialize(); EXPECT_THAT(Env.getValue(*Var), NotNull()); } @@ -225,6 +227,7 @@ TEST_F(EnvironmentTest, IncludeFieldsFromDefaultInitializers) { // Verify that the `X` field of `S` is populated when analyzing the // constructor, even though it is not referenced directly in the constructor. Environment Env(DAContext, *Constructor); + Env.initialize(); auto &Loc = cast<RecordStorageLocation>(Env.createObject(QTy)); EXPECT_THAT(getFieldValue(&Loc, *XDecl, Env), NotNull()); } @@ -268,6 +271,7 @@ TEST_F(EnvironmentTest, InitGlobalVarsFieldFun) { // Verify the global variable is populated when we analyze `Target`. Environment Env(DAContext, *Fun); + Env.initialize(); const auto *GlobalLoc = cast<RecordStorageLocation>(Env.getStorageLocation(*GlobalDecl)); auto *BarVal = getFieldValue(GlobalLoc, *BarDecl, Env); @@ -303,6 +307,7 @@ TEST_F(EnvironmentTest, InitGlobalVarsConstructor) { // Verify the global variable is populated when we analyze `Target`. Environment Env(DAContext, *Ctor); + Env.initialize(); EXPECT_THAT(Env.getValue(*Var), NotNull()); } diff --git a/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp b/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp index 6ba1c2e..84fe675 100644 --- a/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp @@ -18,17 +18,24 @@ namespace { void runDataflow( llvm::StringRef Code, + std::function<llvm::StringMap<QualType>(QualType)> SyntheticFieldCallback, std::function< void(const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &, ASTContext &)> - VerifyResults, - LangStandard::Kind Std = LangStandard::lang_cxx17, - llvm::StringRef TargetFun = "target") { - ASSERT_THAT_ERROR( - checkDataflowWithNoopAnalysis(Code, VerifyResults, - DataflowAnalysisOptions{BuiltinOptions{}}, - Std, TargetFun), - llvm::Succeeded()); + VerifyResults) { + ASSERT_THAT_ERROR(checkDataflowWithNoopAnalysis( + Code, ast_matchers::hasName("target"), VerifyResults, + {BuiltinOptions()}, LangStandard::lang_cxx17, + SyntheticFieldCallback), + llvm::Succeeded()); +} + +const FieldDecl *getFieldNamed(RecordDecl *RD, llvm::StringRef Name) { + for (const FieldDecl *FD : RD->fields()) + if (FD->getName() == Name) + return FD; + assert(false); + return nullptr; } TEST(RecordOpsTest, CopyRecord) { @@ -49,6 +56,13 @@ TEST(RecordOpsTest, CopyRecord) { )"; runDataflow( Code, + [](QualType Ty) -> llvm::StringMap<QualType> { + if (Ty.getAsString() != "S") + return {}; + QualType IntTy = + getFieldNamed(Ty->getAsRecordDecl(), "outer_int")->getType(); + return {{"synth_int", IntTy}}; + }, [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, ASTContext &ASTCtx) { Environment Env = getEnvironmentAtAnnotation(Results, "p").fork(); @@ -68,6 +82,8 @@ TEST(RecordOpsTest, CopyRecord) { EXPECT_NE(S1.getChild(*RefDecl), S2.getChild(*RefDecl)); EXPECT_NE(getFieldValue(&Inner1, *InnerIntDecl, Env), getFieldValue(&Inner2, *InnerIntDecl, Env)); + EXPECT_NE(Env.getValue(S1.getSyntheticField("synth_int")), + Env.getValue(S2.getSyntheticField("synth_int"))); auto *S1Val = cast<RecordValue>(Env.getValue(S1)); auto *S2Val = cast<RecordValue>(Env.getValue(S2)); @@ -82,6 +98,8 @@ TEST(RecordOpsTest, CopyRecord) { EXPECT_EQ(S1.getChild(*RefDecl), S2.getChild(*RefDecl)); EXPECT_EQ(getFieldValue(&Inner1, *InnerIntDecl, Env), getFieldValue(&Inner2, *InnerIntDecl, Env)); + EXPECT_EQ(Env.getValue(S1.getSyntheticField("synth_int")), + Env.getValue(S2.getSyntheticField("synth_int"))); S1Val = cast<RecordValue>(Env.getValue(S1)); S2Val = cast<RecordValue>(Env.getValue(S2)); @@ -109,6 +127,13 @@ TEST(RecordOpsTest, RecordsEqual) { )"; runDataflow( Code, + [](QualType Ty) -> llvm::StringMap<QualType> { + if (Ty.getAsString() != "S") + return {}; + QualType IntTy = + getFieldNamed(Ty->getAsRecordDecl(), "outer_int")->getType(); + return {{"synth_int", IntTy}}; + }, [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, ASTContext &ASTCtx) { Environment Env = getEnvironmentAtAnnotation(Results, "p").fork(); @@ -122,6 +147,9 @@ TEST(RecordOpsTest, RecordsEqual) { auto &S2 = getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "s2"); auto &Inner2 = *cast<RecordStorageLocation>(S2.getChild(*InnerDecl)); + Env.setValue(S1.getSyntheticField("synth_int"), + Env.create<IntegerValue>()); + cast<RecordValue>(Env.getValue(S1)) ->setProperty("prop", Env.getBoolLiteralValue(true)); @@ -162,6 +190,19 @@ TEST(RecordOpsTest, RecordsEqual) { copyRecord(S1, S2, Env); EXPECT_TRUE(recordsEqual(S1, S2, Env)); + // S2 has a different synth_int. + Env.setValue(S2.getSyntheticField("synth_int"), + Env.create<IntegerValue>()); + EXPECT_FALSE(recordsEqual(S1, S2, Env)); + copyRecord(S1, S2, Env); + EXPECT_TRUE(recordsEqual(S1, S2, Env)); + + // S2 doesn't have a value for synth_int. + Env.clearValue(S2.getSyntheticField("synth_int")); + EXPECT_FALSE(recordsEqual(S1, S2, Env)); + copyRecord(S1, S2, Env); + EXPECT_TRUE(recordsEqual(S1, S2, Env)); + // S1 and S2 have the same property with different values. cast<RecordValue>(Env.getValue(S2)) ->setProperty("prop", Env.getBoolLiteralValue(false)); @@ -209,7 +250,7 @@ TEST(TransferTest, CopyRecordFromDerivedToBase) { } )"; runDataflow( - Code, + Code, /*SyntheticFieldCallback=*/{}, [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, ASTContext &ASTCtx) { Environment Env = getEnvironmentAtAnnotation(Results, "p").fork(); diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp b/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp index e24ff25..3726f56 100644 --- a/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp @@ -173,7 +173,8 @@ llvm::Error test::checkDataflowWithNoopAnalysis( void(const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &, ASTContext &)> VerifyResults, - DataflowAnalysisOptions Options, LangStandard::Kind Std) { + DataflowAnalysisOptions Options, LangStandard::Kind Std, + std::function<llvm::StringMap<QualType>(QualType)> SyntheticFieldCallback) { llvm::SmallVector<std::string, 3> ASTBuildArgs = { // -fnodelayed-template-parsing is the default everywhere but on Windows. // Set it explicitly so that tests behave the same on Windows as on other @@ -183,8 +184,10 @@ llvm::Error test::checkDataflowWithNoopAnalysis( std::string(LangStandard::getLangStandardForKind(Std).getName())}; AnalysisInputs<NoopAnalysis> AI( Code, TargetFuncMatcher, - [UseBuiltinModel = Options.BuiltinOpts.has_value()](ASTContext &C, - Environment &Env) { + [UseBuiltinModel = Options.BuiltinOpts.has_value(), + &SyntheticFieldCallback](ASTContext &C, Environment &Env) { + Env.getDataflowAnalysisContext().setSyntheticFieldCallback( + std::move(SyntheticFieldCallback)); return NoopAnalysis( C, DataflowAnalysisOptions{ diff --git a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h b/clang/unittests/Analysis/FlowSensitive/TestingSupport.h index 100d783..95ffcbd 100644 --- a/clang/unittests/Analysis/FlowSensitive/TestingSupport.h +++ b/clang/unittests/Analysis/FlowSensitive/TestingSupport.h @@ -420,7 +420,9 @@ llvm::Error checkDataflowWithNoopAnalysis( ASTContext &)> VerifyResults = [](const auto &, auto &) {}, DataflowAnalysisOptions Options = {BuiltinOptions()}, - LangStandard::Kind Std = LangStandard::lang_cxx17); + LangStandard::Kind Std = LangStandard::lang_cxx17, + std::function<llvm::StringMap<QualType>(QualType)> SyntheticFieldCallback = + {}); /// Returns the `ValueDecl` for the given identifier. /// diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index 76af8ba..73fb406 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -1319,8 +1319,8 @@ protected: llvm::Error Error = checkDataflow<UncheckedOptionalAccessModel>( AnalysisInputs<UncheckedOptionalAccessModel>( SourceCode, std::move(FuncMatcher), - [](ASTContext &Ctx, Environment &) { - return UncheckedOptionalAccessModel(Ctx); + [](ASTContext &Ctx, Environment &Env) { + return UncheckedOptionalAccessModel(Ctx, Env); }) .withPostVisitCFG( [&Diagnostics, |