Author: Martin Braenne Date: 2023-05-15T04:33:29Z New Revision: 48bc71505e03694caac6afb2431ff1157a2382a8
URL: https://github.com/llvm/llvm-project/commit/48bc71505e03694caac6afb2431ff1157a2382a8 DIFF: https://github.com/llvm/llvm-project/commit/48bc71505e03694caac6afb2431ff1157a2382a8.diff LOG: [clang][dataflow] Eliminate `SkipPast::ReferenceThenPointer`. As a replacement, we provide the accessors `getImplicitObjectLocation()` and `getBaseObjectLocation()`, which are higher-level constructs that cover the use cases in which `SkipPast::ReferenceThenPointer` was typically used. Unfortunately, it isn't possible to use these accessors in UncheckedOptionalAccessModel.cpp; I've added a FIXME to the code explaining the details. I initially attempted to resolve the issue as part of this patch, but it turned out to be non-trivial to fix. Instead, I have therefore added a lower-level replacement for `SkipPast::ReferenceThenPointer` that is used only within this file. The wider context of this change is that `SkipPast` will be going away entirely. See also the RFC at https://discourse.llvm.org/t/70086. Reviewed By: ymandel, gribozavr2 Differential Revision: https://reviews.llvm.org/D149838 Added: Modified: clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp clang/lib/Analysis/FlowSensitive/Transfer.cpp clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h index c23e0db7f82d3..d734ae5c66fe8 100644 --- a/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h @@ -46,9 +46,6 @@ enum class SkipPast { 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 @@ class Environment { 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 index 0d269f503f4eb..7b1944f273cf0 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -782,11 +782,6 @@ StorageLocation &Environment::skip(StorageLocation &Loc, SkipPast SP) const { if (auto *Val = dyn_cast_or_null<ReferenceValue>(getValue(Loc))) return Val->getReferentLoc(); return Loc; - case SkipPast::ReferenceThenPointer: - StorageLocation &LocPastRef = skip(Loc, SkipPast::Reference); - if (auto *Val = dyn_cast_or_null<PointerValue>(getValue(LocPastRef))) - return Val->getPointeeLoc(); - return LocPastRef; } llvm_unreachable("bad SkipPast kind"); } @@ -828,5 +823,39 @@ void Environment::dump() const { 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<PointerValue>(Env.getValue(*Loc))) + return &cast<AggregateStorageLocation>(Val->getPointeeLoc()); + return nullptr; + } + return cast<AggregateStorageLocation>(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<PointerValue>(Env.getValue(*Loc))) + return &cast<AggregateStorageLocation>(Val->getPointeeLoc()); + return nullptr; + } + return cast<AggregateStorageLocation>(Loc); +} + } // namespace dataflow } // namespace clang diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index e306b55753752..b46a57f47f982 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -372,10 +372,26 @@ bool isNonEmptyOptional(const Value &OptionalVal, const Environment &Env) { 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<PointerValue>(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<PointerValue>(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 @@ void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr, 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 @@ void transferValueOrImpl(const clang::Expr *ValueOrPredExpr, ->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 transferCallReturningOptional(const CallExpr *E, 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 @@ void transferNulloptAssignment(const CXXOperatorCallExpr *E, 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 @@ void transferSwapCall(const CXXMemberCallExpr *E, 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 @@ auto buildTransferMatchSwitch() { }) // optional::operator*, optional::operator-> + // FIXME: This does something slightly strange for `operator->`. + // `transferUnwrapCall()` may create a new value of type `T` for the + // `optional<T>`, 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<CallExpr>(valueOperatorCall(std::nullopt), [](const CallExpr *E, const MatchFinder::MatchResult &, @@ -837,8 +867,7 @@ auto buildTransferMatchSwitch() { std::vector<SourceLocation> 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<BoolValue>(Prop)) { if (Env.flowConditionImplies(*HasValueVal)) diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp index 50eb7e9011bda..142dd2f21a5c6 100644 --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -542,10 +542,7 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> { } } - // 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<AggregateStorageLocation>( - 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 index a8b0c6bd6a087..83b9f33493d0b 100644 --- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp @@ -372,8 +372,7 @@ class SpecialBoolAnalysis final 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 @@ class OptionalIntAnalysis final 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()); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
