diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h @@ -46,9 +46,6 @@ None, /// An optional reference should be skipped past. Reference, - /// An optional reference should be skipped past, then an optional pointer - /// should be skipped past. - ReferenceThenPointer, }; /// Indicates the result of a tentative comparison. @@ -477,6 +474,19 @@ AtomicBoolValue *FlowConditionToken; }; +/// Returns the storage location for the implicit object of a +/// `CXXMemberCallExpr`, or null if none is defined in the environment. +/// Dereferences the pointer if the member call expression was written using +/// `->`. +AggregateStorageLocation * +getImplicitObjectLocation(const CXXMemberCallExpr &MCE, const Environment &Env); + +/// Returns the storage location for the base object of a `MemberExpr`, or null +/// if none is defined in the environment. Dereferences the pointer if the +/// member expression was written using `->`. +AggregateStorageLocation *getBaseObjectLocation(const MemberExpr &ME, + const Environment &Env); + } // namespace dataflow } // namespace clang diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -782,11 +782,6 @@ if (auto *Val = dyn_cast_or_null(getValue(Loc))) return Val->getReferentLoc(); return Loc; - case SkipPast::ReferenceThenPointer: - StorageLocation &LocPastRef = skip(Loc, SkipPast::Reference); - if (auto *Val = dyn_cast_or_null(getValue(LocPastRef))) - return Val->getPointeeLoc(); - return LocPastRef; } llvm_unreachable("bad SkipPast kind"); } @@ -828,5 +823,39 @@ dump(llvm::dbgs()); } +AggregateStorageLocation * +getImplicitObjectLocation(const CXXMemberCallExpr &MCE, + const Environment &Env) { + Expr *ImplicitObject = MCE.getImplicitObjectArgument(); + if (ImplicitObject == nullptr) + return nullptr; + StorageLocation *Loc = + Env.getStorageLocation(*ImplicitObject, SkipPast::Reference); + if (Loc == nullptr) + return nullptr; + if (ImplicitObject->getType()->isPointerType()) { + if (auto *Val = cast_or_null(Env.getValue(*Loc))) + return &cast(Val->getPointeeLoc()); + return nullptr; + } + return cast(Loc); +} + +AggregateStorageLocation *getBaseObjectLocation(const MemberExpr &ME, + const Environment &Env) { + Expr *Base = ME.getBase(); + if (Base == nullptr) + return nullptr; + StorageLocation *Loc = Env.getStorageLocation(*Base, SkipPast::Reference); + if (Loc == nullptr) + return nullptr; + if (ME.isArrow()) { + if (auto *Val = cast_or_null(Env.getValue(*Loc))) + return &cast(Val->getPointeeLoc()); + return nullptr; + } + return cast(Loc); +} + } // namespace dataflow } // namespace clang diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -372,10 +372,26 @@ return HasValueVal != nullptr && Env.flowConditionImplies(*HasValueVal); } +StorageLocation *maybeSkipPointer(StorageLocation *Loc, + const Environment &Env) { + if (Loc == nullptr) + return nullptr; + if (auto *Val = dyn_cast_or_null(Env.getValue(*Loc))) + return &Val->getPointeeLoc(); + return Loc; +} + +Value *getValueBehindPossiblePointer(const Expr &E, const Environment &Env) { + Value *Val = Env.getValue(E, SkipPast::Reference); + if (auto *PointerVal = dyn_cast_or_null(Val)) + return Env.getValue(PointerVal->getPointeeLoc()); + return Val; +} + void transferUnwrapCall(const Expr *UnwrapExpr, const Expr *ObjectExpr, LatticeTransferState &State) { if (auto *OptionalVal = - State.Env.getValue(*ObjectExpr, SkipPast::ReferenceThenPointer)) { + getValueBehindPossiblePointer(*ObjectExpr, State.Env)) { if (State.Env.getStorageLocation(*UnwrapExpr, SkipPast::None) == nullptr) if (auto *Loc = maybeInitializeOptionalValueMember( UnwrapExpr->getType(), *OptionalVal, State.Env)) @@ -396,8 +412,8 @@ const MatchFinder::MatchResult &, LatticeTransferState &State) { if (auto *HasValueVal = getHasValue( - State.Env, State.Env.getValue(*CallExpr->getImplicitObjectArgument(), - SkipPast::ReferenceThenPointer))) { + State.Env, getValueBehindPossiblePointer( + *CallExpr->getImplicitObjectArgument(), State.Env))) { auto &CallExprLoc = State.Env.createStorageLocation(*CallExpr); State.Env.setValue(CallExprLoc, *HasValueVal); State.Env.setStorageLocation(*CallExpr, CallExprLoc); @@ -419,8 +435,7 @@ ->getImplicitObjectArgument(); auto *HasValueVal = getHasValue( - State.Env, - State.Env.getValue(*ObjectArgumentExpr, SkipPast::ReferenceThenPointer)); + State.Env, getValueBehindPossiblePointer(*ObjectArgumentExpr, State.Env)); if (HasValueVal == nullptr) return; @@ -472,8 +487,8 @@ void assignOptionalValue(const Expr &E, Environment &Env, BoolValue &HasValueVal) { - if (auto *OptionalLoc = - Env.getStorageLocation(E, SkipPast::ReferenceThenPointer)) { + if (auto *OptionalLoc = maybeSkipPointer( + Env.getStorageLocation(E, SkipPast::Reference), Env)) { Env.setValue(*OptionalLoc, createOptionalValue(Env, HasValueVal)); } } @@ -550,13 +565,11 @@ transferAssignment(E, State.Env.getBoolLiteralValue(false), State); } -void transferSwap(const Expr &E1, SkipPast E1Skip, const Expr &E2, +void transferSwap(StorageLocation *Loc1, StorageLocation *Loc2, Environment &Env) { // We account for cases where one or both of the optionals are not modeled, // either lacking associated storage locations, or lacking values associated // to such storage locations. - auto *Loc1 = Env.getStorageLocation(E1, E1Skip); - auto *Loc2 = Env.getStorageLocation(E2, SkipPast::Reference); if (Loc1 == nullptr) { if (Loc2 != nullptr) @@ -590,14 +603,20 @@ const MatchFinder::MatchResult &, LatticeTransferState &State) { assert(E->getNumArgs() == 1); - transferSwap(*E->getImplicitObjectArgument(), SkipPast::ReferenceThenPointer, - *E->getArg(0), State.Env); + transferSwap(maybeSkipPointer( + State.Env.getStorageLocation(*E->getImplicitObjectArgument(), + SkipPast::Reference), + State.Env), + State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference), + State.Env); } void transferStdSwapCall(const CallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { assert(E->getNumArgs() == 2); - transferSwap(*E->getArg(0), SkipPast::Reference, *E->getArg(1), State.Env); + transferSwap(State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference), + State.Env.getStorageLocation(*E->getArg(1), SkipPast::Reference), + State.Env); } void transferStdForwardCall(const CallExpr *E, const MatchFinder::MatchResult &, @@ -756,6 +775,17 @@ }) // optional::operator*, optional::operator-> + // FIXME: This does something slightly strange for `operator->`. + // `transferUnwrapCall()` may create a new value of type `T` for the + // `optional`, and it associates that value with `E`. In the case of + // `operator->`, `E` is a pointer. As a result, we associate an + // expression of pointer type with a storage location of non-pointer type + // `T`. This can confound other code that expects expressions of + // pointer type to be associated with `PointerValue`s, such as the + // centrally provided accessors `getImplicitObjectLocation()` and + // `getBaseObjectLocation()`, and this is the reason we need to use our + // own 'maybeSkipPointer()` and `getValueBehindPossiblePointer()` instead + // of these accessors. .CaseOfCFGStmt(valueOperatorCall(std::nullopt), [](const CallExpr *E, const MatchFinder::MatchResult &, @@ -837,8 +867,7 @@ std::vector diagnoseUnwrapCall(const Expr *UnwrapExpr, const Expr *ObjectExpr, const Environment &Env) { - if (auto *OptionalVal = - Env.getValue(*ObjectExpr, SkipPast::ReferenceThenPointer)) { + if (auto *OptionalVal = getValueBehindPossiblePointer(*ObjectExpr, Env)) { auto *Prop = OptionalVal->getProperty("has_value"); if (auto *HasValueVal = cast_or_null(Prop)) { if (Env.flowConditionImplies(*HasValueVal)) diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -542,10 +542,7 @@ } } - // The receiver can be either a value or a pointer to a value. Skip past the - // indirection to handle both cases. - auto *BaseLoc = cast_or_null( - Env.getStorageLocation(*S->getBase(), SkipPast::ReferenceThenPointer)); + AggregateStorageLocation *BaseLoc = getBaseObjectLocation(*S, Env); if (BaseLoc == nullptr) return; diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp --- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp @@ -372,8 +372,7 @@ auto *Object = E->getImplicitObjectArgument(); assert(Object != nullptr); - auto *ObjectLoc = - Env.getStorageLocation(*Object, SkipPast::ReferenceThenPointer); + auto *ObjectLoc = getImplicitObjectLocation(*E, Env); assert(ObjectLoc != nullptr); auto &ConstructorVal = *Env.createValue(Object->getType()); @@ -532,8 +531,7 @@ auto *Object = E->getArg(0); assert(Object != nullptr); - auto *ObjectLoc = - Env.getStorageLocation(*Object, SkipPast::ReferenceThenPointer); + auto *ObjectLoc = Env.getStorageLocation(*Object, SkipPast::Reference); assert(ObjectLoc != nullptr); auto &ConstructorVal = *Env.createValue(Object->getType());