diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h index c5f14cfcd4f7b..1dffbe8a09c36 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 +auto createAnalysis(ASTContext &ASTCtx, Environment &Env) + -> decltype(AnalysisT(ASTCtx, Env)) { + return AnalysisT(ASTCtx, Env); +} +template +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> diagnoseFunction( const WatchedLiteralsSolver *Solver = OwnedSolver.get(); DataflowAnalysisContext AnalysisContext(std::move(OwnedSolver)); Environment Env(AnalysisContext, FuncDecl); - AnalysisT Analysis(ASTCtx); + AnalysisT Analysis = createAnalysis(ASTCtx, Env); llvm::SmallVector 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 c1aa8484817a9..20e45cc27b01f 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; /// 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 @@ class DataflowAnalysisContext { /*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(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 @@ class DataflowAnalysisContext { /// context. FieldSet getModeledFields(QualType Type); + /// Returns the names and types of the synthetic fields for the given record + /// type. + llvm::StringMap getSyntheticFields(QualType Type) { + assert(Type->isRecordType()); + if (SyntheticFieldCallback) + return SyntheticFieldCallback(Type); + return {}; + } + private: friend class Environment; @@ -250,6 +291,11 @@ class DataflowAnalysisContext { FieldSet ModeledFields; std::unique_ptr LogOwner; // If created via flags. + + std::function(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 7c1f549109632..8502f4da7e541 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h @@ -166,6 +166,10 @@ class Environment { /// with a symbolic representation of the `this` pointee. Environment(DataflowAnalysisContext &DACtx, const DeclContext &DeclCtx); + /// Assigns storage locations and values to all global variables, fields + /// and functions referenced in `FuncDecl`. `FuncDecl` must have a body. + void initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl); + /// 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 +287,15 @@ class Environment { /// 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 +379,8 @@ class Environment { /// 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 +583,9 @@ class Environment { return dyn_cast(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`. @@ -629,10 +645,7 @@ class Environment { void pushCallInternal(const FunctionDecl *FuncDecl, ArrayRef Args); - /// Assigns storage locations and values to all global variables, fields - /// and functions referenced in `FuncDecl`. `FuncDecl` must have a body. - void initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl); - +private: // `DACtx` is not null and not owned by this object. DataflowAnalysisContext *DACtx; diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h index aeaf75b215422..09eb8b9382261 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 { 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 @@ class UncheckedOptionalAccessModel 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> TransferMatchSwitch; }; diff --git a/clang/include/clang/Analysis/FlowSensitive/RecordOps.h b/clang/include/clang/Analysis/FlowSensitive/RecordOps.h index f007a947a82f1..7b87840d626b4 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 89d2bbfbb69f9..b7203378189d6 100644 --- a/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h +++ b/clang/include/clang/Analysis/FlowSensitive/StorageLocation.h @@ -73,7 +73,13 @@ class ScalarStorageLocation final : public StorageLocation { /// /// 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 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 @@ class ScalarStorageLocation final : public StorageLocation { class RecordStorageLocation final : public StorageLocation { public: using FieldToLoc = llvm::DenseMap; + using SyntheticFieldMap = llvm::StringMap; - 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 @@ class RecordStorageLocation final : public StorageLocation { 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 + 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 @@ class RecordStorageLocation final : public StorageLocation { private: FieldToLoc Children; + SyntheticFieldMap SyntheticFields; }; } // namespace dataflow diff --git a/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp b/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp index 0a2fcd4ad695a..fa114979c8e32 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(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(Type); } +// Returns the keys for a given `StringMap`. +// Can't use `StringSet` as the return type as it doesn't support `operator==`. +template +static llvm::DenseSet getKeys(const llvm::StringMap &Map) { + return llvm::DenseSet(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(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(Field))) + return false; + return true; +} diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp index 1a38be9c1374f..983375da9cc3a 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -157,12 +157,25 @@ static Value &widenDistinctValues(QualType Type, Value &Prev, Environment &CurrentEnv, Environment::ValueModel &Model) { // Boolean-model widening. - if (isa(&Prev)) { - assert(isa(Current)); - // Widen to Top, because we know they are different values. If previous was - // already Top, re-use that to (implicitly) indicate that no change occured. + if (auto *PrevBool = dyn_cast(&Prev)) { + // If previous value was already Top, re-use that to (implicitly) indicate + // that no change occurred. if (isa(Prev)) return Prev; + + // We may need to widen to Top, but before we do so, check whether both + // values are implied to be either true or false in the current environment. + // In that case, we can simply return a literal instead. + auto &CurBool = cast(Current); + bool TruePrev = PrevEnv.proves(PrevBool->formula()); + bool TrueCur = CurrentEnv.proves(CurBool.formula()); + if (TruePrev && TrueCur) + return CurrentEnv.getBoolLiteralValue(true); + if (!TruePrev && !TrueCur && + PrevEnv.proves(PrevEnv.arena().makeNot(PrevBool->formula())) && + CurrentEnv.proves(CurrentEnv.arena().makeNot(CurBool.formula()))) + return CurrentEnv.getBoolLiteralValue(false); + return CurrentEnv.makeTopBoolValue(); } @@ -354,6 +367,16 @@ 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); +} + // FIXME: Add support for resetting globals after function calls to enable // the implementation of sound analyses. void Environment::initFieldsGlobalsAndFuncs(const FunctionDecl *FuncDecl) { @@ -403,59 +426,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(&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(&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(DeclCtx.getNonClosureAncestor()); - QualType ThisPointeeType = - SurroundingMethodDecl->getFunctionObjectParameterType(); - ThisPointeeLoc = - &cast(createValue(ThisPointeeType))->getLoc(); - } - } - } else if (MethodDecl->isImplicitObjectMemberFunction()) { - QualType ThisPointeeType = MethodDecl->getFunctionObjectParameterType(); - ThisPointeeLoc = - &cast(createValue(ThisPointeeType))->getLoc(); - } - } -} - bool Environment::canDescend(unsigned MaxDepth, const DeclContext *Callee) const { return CallStack.size() <= MaxDepth && !llvm::is_contained(CallStack, Callee); @@ -714,10 +690,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()); @@ -839,8 +811,16 @@ Value *Environment::createValueUnlessSelfReferential( CreatedValuesCount)}); } - RecordStorageLocation &Loc = - arena().create(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(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 8329367098b1d..2cf3f321fcf45 100644 --- a/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp +++ b/clang/lib/Analysis/FlowSensitive/HTMLLogger.cpp @@ -136,6 +136,10 @@ class ModelDumper { if (Value *Val = Env.getValue(*Child.second)) dump(*Val); }); + + for (const auto &Prop : RLoc->synthetic_fields()) + JOS.attributeObject(("sf:" + Prop.first()).str(), + [&] { dump(*Prop.second); }); } } diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 55d0713639d90..69ac2c2b82cff 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(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(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(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(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(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(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 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(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(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(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(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( + 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(*Loc)); - } - } + if (auto *OptionalLoc = cast_or_null( + getLocBehindPossiblePointer(*ObjectExpr, State.Env))) + State.Env.setValue( + *UnwrapExpr, State.Env.create(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(ValueOrCallID) - ->getImplicitObjectArgument(); + const auto *MCE = + Result.Nodes.getNodeAs(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` with argument of // type `optional` such that `T` is constructible from `U`. - if (auto *HasValueVal = getHasValue(State.Env, State.Env.getValue(E))) + auto *Loc = + cast_or_null(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( + Env.getStorageLocation(*CmpExpr->getArg(0))); + if (auto *LHasVal = getHasValue(Env, Arg0Loc)) { + auto *Arg1Loc = cast_or_null( + 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(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(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 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() - // Attach a symbolic "has_value" state to optional values that we see for - // the first time. - .CaseOfCFGStmt( - expr(anyOf(declRefExpr(), memberExpr()), hasOptionalType()), - initializeOptionalReference) - // make_optional .CaseOfCFGStmt(isMakeOptionalCall(), transferMakeOptionalCall) @@ -779,14 +665,6 @@ auto buildTransferMatchSwitch() { constructOptionalValue(*E, State.Env, State.Env.getBoolLiteralValue(true)); }) - // nullopt_t::nullopt_t - .CaseOfCFGStmt( - isNulloptConstructor(), - [](const CXXConstructExpr *E, const MatchFinder::MatchResult &, - LatticeTransferState &State) { - constructOptionalValue(*E, State.Env, - State.Env.getBoolLiteralValue(false)); - }) // optional::optional(nullopt_t) .CaseOfCFGStmt( isOptionalNulloptConstructor(), @@ -887,18 +765,32 @@ auto buildTransferMatchSwitch() { // Comparisons (==, !=): .CaseOfCFGStmt( - isComparisonOperatorCall(hasAnyOptionalType(), hasAnyOptionalType()), + isComparisonOperatorCall(hasOptionalType(), hasOptionalType()), transferOptionalAndOptionalCmp) .CaseOfCFGStmt( - isComparisonOperatorCall(hasOptionalType(), - unless(hasAnyOptionalType())), + isComparisonOperatorCall(hasOptionalType(), hasNulloptType()), + [](const clang::CXXOperatorCallExpr *Cmp, + const MatchFinder::MatchResult &, LatticeTransferState &State) { + transferOptionalAndNulloptCmp(Cmp, Cmp->getArg(0), State.Env); + }) + .CaseOfCFGStmt( + isComparisonOperatorCall(hasNulloptType(), hasOptionalType()), + [](const clang::CXXOperatorCallExpr *Cmp, + const MatchFinder::MatchResult &, LatticeTransferState &State) { + transferOptionalAndNulloptCmp(Cmp, Cmp->getArg(1), State.Env); + }) + .CaseOfCFGStmt( + isComparisonOperatorCall( + hasOptionalType(), + unless(anyOf(hasOptionalType(), hasNulloptType()))), [](const clang::CXXOperatorCallExpr *Cmp, const MatchFinder::MatchResult &, LatticeTransferState &State) { transferOptionalAndValueCmp(Cmp, Cmp->getArg(0), State.Env); }) .CaseOfCFGStmt( - 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 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( + getLocBehindPossiblePointer(*ObjectExpr, Env))) { + auto *Prop = Env.getValue(locForHasValue(*OptionalLoc)); if (auto *HasValueVal = cast_or_null(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(OptionalTy->getAsCXXRecordDecl()); + return CTSD->getTemplateArgs()[0].getAsType(); +} + +UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx, + Environment &Env) : DataflowAnalysis(Ctx), - TransferMatchSwitch(buildTransferMatchSwitch()) {} + TransferMatchSwitch(buildTransferMatchSwitch()) { + Env.getDataflowAnalysisContext().setSyntheticFieldCallback( + [&Ctx](QualType Ty) -> llvm::StringMap { + 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(Prev.getProperty("has_value"))) { - if (isa(PrevHasVal)) - return &Prev; - } - if (auto *CurrentHasVal = - cast_or_null(Current.getProperty("has_value"))) { - if (isa(CurrentHasVal)) - return &Current; - } - return &createOptionalValue(cast(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 38638f8a9e589..47a713472bc39 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, PropLocSrc] : Src.synthetic_fields()) { + if (PropLocSrc->getType()->isRecordType()) { + copyRecord(*cast(PropLocSrc), + cast(Dst.getSyntheticField(Name)), Env); + } else { + if (Value *Val = Env.getValue(*PropLocSrc)) + Env.setValue(Dst.getSyntheticField(Name), *Val); + else + Env.clearValue(Dst.getSyntheticField(Name)); + } + } + RecordValue *SrcVal = cast_or_null(Env.getValue(Src)); RecordValue *DstVal = cast_or_null(Env.getValue(Dst)); @@ -101,6 +113,18 @@ bool clang::dataflow::recordsEqual(const RecordStorageLocation &Loc1, } } + for (const auto &[Name, PropLoc1] : Loc1.synthetic_fields()) { + if (PropLoc1->getType()->isRecordType()) { + if (!recordsEqual( + *cast(PropLoc1), Env1, + cast(Loc2.getSyntheticField(Name)), Env2)) + return false; + } else if (Env1.getValue(*PropLoc1) != + Env2.getValue(Loc2.getSyntheticField(Name))) { + return false; + } + } + llvm::StringMap Props1, Props2; if (RecordValue *Val1 = cast_or_null(Env1.getValue(Loc1))) diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp index 4343af7900813..bbf5f12359bc7 100644 --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -703,20 +703,18 @@ class TransferVisitor : public ConstStmtVisitor { // `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(Field))) - return false; - return true; - }()); - - auto &Loc = - Env.getDataflowAnalysisContext().arena().create( - 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(Loc); Env.setValue(Loc, RecordVal); diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp index ade8c84f19366..462a45ef01e17 100644 --- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp +++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp @@ -492,6 +492,56 @@ transferCFGBlock(const CFGBlock &Block, AnalysisContext &AC, return State; } +static Environment initializeEnvironment(const Environment &InitEnv) { + Environment ResultEnv = InitEnv.fork(); + + const DeclContext *DeclCtx = ResultEnv.getDeclCtx(); + if (DeclCtx == nullptr) + return ResultEnv; + + if (const auto *FuncDecl = dyn_cast(DeclCtx)) { + assert(FuncDecl->getBody() != nullptr); + + ResultEnv.initFieldsGlobalsAndFuncs(FuncDecl); + + for (const auto *ParamDecl : FuncDecl->parameters()) { + assert(ParamDecl != nullptr); + ResultEnv.setStorageLocation(*ParamDecl, + ResultEnv.createObject(*ParamDecl, nullptr)); + } + } + + if (const auto *MethodDecl = dyn_cast(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); + ResultEnv.setStorageLocation( + *VarDecl, ResultEnv.createObject(*VarDecl, nullptr)); + } else if (Capture.capturesThis()) { + const auto *SurroundingMethodDecl = + cast(DeclCtx->getNonClosureAncestor()); + QualType ThisPointeeType = + SurroundingMethodDecl->getFunctionObjectParameterType(); + ResultEnv.setThisPointeeStorageLocation( + cast(ResultEnv.createValue(ThisPointeeType)) + ->getLoc()); + } + } + } else if (MethodDecl->isImplicitObjectMemberFunction()) { + QualType ThisPointeeType = MethodDecl->getFunctionObjectParameterType(); + ResultEnv.setThisPointeeStorageLocation( + cast(ResultEnv.createValue(ThisPointeeType))->getLoc()); + } + } + + return ResultEnv; +} + llvm::Expected>> runTypeErasedDataflowAnalysis( const ControlFlowContext &CFCtx, TypeErasedDataflowAnalysis &Analysis, @@ -501,6 +551,13 @@ runTypeErasedDataflowAnalysis( PostVisitCFG) { PrettyStackTraceAnalysis CrashInfo(CFCtx, "runTypeErasedDataflowAnalysis"); + auto MaybeStartingEnv = + InitEnv.callStackSize() == 1 + ? std::make_optional(initializeEnvironment(InitEnv)) + : std::nullopt; + const Environment &StartingEnv = + MaybeStartingEnv ? *MaybeStartingEnv : InitEnv; + const clang::CFG &CFG = CFCtx.getCFG(); PostOrderCFGView POV(&CFG); ForwardDataflowWorklist Worklist(CFG, &POV); @@ -511,10 +568,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 ff6cbec5d20b7..595d05e2ba5d1 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.initFieldsGlobalsAndFuncs(Fun); auto &SLoc = cast(Env.createObject(Ty)); PointerValue *PV = cast_or_null(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.initFieldsGlobalsAndFuncs(Fun); 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.initFieldsGlobalsAndFuncs(Constructor); auto &Loc = cast(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.initFieldsGlobalsAndFuncs(Fun); const auto *GlobalLoc = cast(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.initFieldsGlobalsAndFuncs(Ctor); EXPECT_THAT(Env.getValue(*Var), NotNull()); } diff --git a/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp b/clang/unittests/Analysis/FlowSensitive/RecordOpsTest.cpp index 6ba1c2ebd833d..84fe675c32c2d 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(QualType)> SyntheticFieldCallback, std::function< void(const llvm::StringMap> &, 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 { + if (Ty.getAsString() != "S") + return {}; + QualType IntTy = + getFieldNamed(Ty->getAsRecordDecl(), "outer_int")->getType(); + return {{"synth_int", IntTy}}; + }, [](const llvm::StringMap> &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(Env.getValue(S1)); auto *S2Val = cast(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(Env.getValue(S1)); S2Val = cast(Env.getValue(S2)); @@ -109,6 +127,13 @@ TEST(RecordOpsTest, RecordsEqual) { )"; runDataflow( Code, + [](QualType Ty) -> llvm::StringMap { + if (Ty.getAsString() != "S") + return {}; + QualType IntTy = + getFieldNamed(Ty->getAsRecordDecl(), "outer_int")->getType(); + return {{"synth_int", IntTy}}; + }, [](const llvm::StringMap> &Results, ASTContext &ASTCtx) { Environment Env = getEnvironmentAtAnnotation(Results, "p").fork(); @@ -122,6 +147,9 @@ TEST(RecordOpsTest, RecordsEqual) { auto &S2 = getLocForDecl(ASTCtx, Env, "s2"); auto &Inner2 = *cast(S2.getChild(*InnerDecl)); + Env.setValue(S1.getSyntheticField("synth_int"), + Env.create()); + cast(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()); + 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(Env.getValue(S2)) ->setProperty("prop", Env.getBoolLiteralValue(false)); @@ -209,7 +250,7 @@ TEST(TransferTest, CopyRecordFromDerivedToBase) { } )"; runDataflow( - Code, + Code, /*SyntheticFieldCallback=*/{}, [](const llvm::StringMap> &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 e24ff25cb8292..3726f56fc824d 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> &, ASTContext &)> VerifyResults, - DataflowAnalysisOptions Options, LangStandard::Kind Std) { + DataflowAnalysisOptions Options, LangStandard::Kind Std, + std::function(QualType)> SyntheticFieldCallback) { llvm::SmallVector 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 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 100d78378695d..95ffcbd6f322e 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(QualType)> SyntheticFieldCallback = + {}); /// Returns the `ValueDecl` for the given identifier. /// diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp index ade0d202ced2f..8da55953a3298 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -4167,6 +4167,34 @@ TEST(TransferTest, LoopWithShortCircuitedConditionConverges) { ASSERT_THAT_ERROR(checkDataflowWithNoopAnalysis(Code), llvm::Succeeded()); } +TEST(TransferTest, LoopCanProveInvariantForBoolean) { + // Check that we can prove `b` is always false in the loop. + // This test exercises the logic in `widenDistinctValues()` that preserves + // information if the boolean can be proved to be either true or false in both + // the previous and current iteration. + std::string Code = R"cc( + int return_int(); + void target() { + bool b = return_int() == 0; + if (b) return; + while (true) { + b; + // [[p]] + b = return_int() == 0; + if (b) return; + } + } + )cc"; + runDataflow( + Code, + [](const llvm::StringMap> &Results, + ASTContext &ASTCtx) { + const Environment &Env = getEnvironmentAtAnnotation(Results, "p"); + auto &BVal = getValueForDecl(ASTCtx, Env, "b"); + EXPECT_TRUE(Env.proves(Env.arena().makeNot(BVal.formula()))); + }); +} + TEST(TransferTest, DoesNotCrashOnUnionThisExpr) { std::string Code = R"( union Union { diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp index 76af8baba8518..73fb4063d92be 100644 --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -1319,8 +1319,8 @@ class UncheckedOptionalAccessTest llvm::Error Error = checkDataflow( AnalysisInputs( SourceCode, std::move(FuncMatcher), - [](ASTContext &Ctx, Environment &) { - return UncheckedOptionalAccessModel(Ctx); + [](ASTContext &Ctx, Environment &Env) { + return UncheckedOptionalAccessModel(Ctx, Env); }) .withPostVisitCFG( [&Diagnostics,