Author: Martin Braenne Date: 2023-07-10T06:45:55Z New Revision: f653d14065a362c98114082c4e9a3b1ede7a90f5
URL: https://github.com/llvm/llvm-project/commit/f653d14065a362c98114082c4e9a3b1ede7a90f5 DIFF: https://github.com/llvm/llvm-project/commit/f653d14065a362c98114082c4e9a3b1ede7a90f5.diff LOG: [clang][dataflow] Various refactorings to UncheckedOptionalAccessModel. These are intended to ease an upcoming change that will eliminate the duplication between `AggregateStorageLocation` and `StructValue` (see https://discourse.llvm.org/t/70086 for details), but many of the changes also have value in their own right. Depends On D154586 Reviewed By: ymandel, gribozavr2 Differential Revision: https://reviews.llvm.org/D154597 Added: Modified: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp Removed: ################################################################################ diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index a239d54674e96c..91aa9d11e751cd 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -255,12 +255,22 @@ void setHasValue(Value &OptionalVal, BoolValue &HasValueVal) { /// Creates a symbolic value for an `optional` value using `HasValueVal` as the /// symbolic value of its "has_value" property. -StructValue &createOptionalValue(Environment &Env, BoolValue &HasValueVal) { +StructValue &createOptionalValue(BoolValue &HasValueVal, Environment &Env) { auto &OptionalVal = Env.create<StructValue>(); setHasValue(OptionalVal, HasValueVal); return OptionalVal; } +/// Creates a symbolic value for an `optional` value at an existing storage +/// location. Uses `HasValueVal` as the symbolic value of the "has_value" +/// property. +StructValue &createOptionalValue(AggregateStorageLocation &Loc, + BoolValue &HasValueVal, Environment &Env) { + StructValue &OptionalVal = createOptionalValue(HasValueVal, Env); + Env.setValue(Loc, OptionalVal); + 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) { @@ -422,15 +432,6 @@ 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)) @@ -467,7 +468,7 @@ void transferMakeOptionalCall(const CallExpr *E, auto &Loc = State.Env.createStorageLocation(*E); State.Env.setStorageLocation(*E, Loc); State.Env.setValue( - Loc, createOptionalValue(State.Env, State.Env.getBoolLiteralValue(true))); + Loc, createOptionalValue(State.Env.getBoolLiteralValue(true), State.Env)); } void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr, @@ -544,15 +545,12 @@ void transferCallReturningOptional(const CallExpr *E, auto &Loc = State.Env.createStorageLocation(*E); State.Env.setStorageLocation(*E, Loc); State.Env.setValue( - Loc, createOptionalValue(State.Env, State.Env.makeAtomicBoolValue())); + Loc, createOptionalValue(State.Env.makeAtomicBoolValue(), State.Env)); } -void assignOptionalValue(const Expr &E, Environment &Env, - BoolValue &HasValueVal) { - if (auto *OptionalLoc = maybeSkipPointer( - Env.getStorageLocation(E, SkipPast::Reference), Env)) { - Env.setValue(*OptionalLoc, createOptionalValue(Env, HasValueVal)); - } +void constructOptionalValue(const Expr &E, Environment &Env, + BoolValue &HasValueVal) { + Env.setValueStrict(E, createOptionalValue(HasValueVal, Env)); } /// Returns a symbolic value for the "has_value" property of an `optional<T>` @@ -590,25 +588,23 @@ void transferValueOrConversionConstructor( LatticeTransferState &State) { assert(E->getNumArgs() > 0); - assignOptionalValue(*E, State.Env, - valueOrConversionHasValue(*E->getConstructor(), - *E->getArg(0), MatchRes, - State)); + constructOptionalValue(*E, State.Env, + valueOrConversionHasValue(*E->getConstructor(), + *E->getArg(0), MatchRes, + State)); } void transferAssignment(const CXXOperatorCallExpr *E, BoolValue &HasValueVal, LatticeTransferState &State) { assert(E->getNumArgs() > 0); - auto *OptionalLoc = - State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference); - if (OptionalLoc == nullptr) - return; - - State.Env.setValue(*OptionalLoc, createOptionalValue(State.Env, HasValueVal)); + if (auto *Loc = cast<AggregateStorageLocation>( + State.Env.getStorageLocationStrict(*E->getArg(0)))) { + createOptionalValue(*Loc, HasValueVal, State.Env); - // Assign a storage location for the whole expression. - State.Env.setStorageLocation(*E, *OptionalLoc); + // Assign a storage location for the whole expression. + State.Env.setStorageLocationStrict(*E, *Loc); + } } void transferValueOrConversionAssignment( @@ -627,19 +623,19 @@ void transferNulloptAssignment(const CXXOperatorCallExpr *E, transferAssignment(E, State.Env.getBoolLiteralValue(false), State); } -void transferSwap(StorageLocation *Loc1, StorageLocation *Loc2, - Environment &Env) { +void transferSwap(AggregateStorageLocation *Loc1, + AggregateStorageLocation *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. if (Loc1 == nullptr) { if (Loc2 != nullptr) - Env.setValue(*Loc2, createOptionalValue(Env, Env.makeAtomicBoolValue())); + createOptionalValue(*Loc2, Env.makeAtomicBoolValue(), Env); return; } if (Loc2 == nullptr) { - Env.setValue(*Loc1, createOptionalValue(Env, Env.makeAtomicBoolValue())); + createOptionalValue(*Loc1, Env.makeAtomicBoolValue(), Env); return; } @@ -649,36 +645,35 @@ void transferSwap(StorageLocation *Loc1, StorageLocation *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. - auto *Val1 = Env.getValue(*Loc1); - if (Val1 == nullptr) - Val1 = &createOptionalValue(Env, Env.makeAtomicBoolValue()); + BoolValue *BoolVal1 = getHasValue(Env, Env.getValue(*Loc1)); + if (BoolVal1 == nullptr) + BoolVal1 = &Env.makeAtomicBoolValue(); - auto *Val2 = Env.getValue(*Loc2); - if (Val2 == nullptr) - Val2 = &createOptionalValue(Env, Env.makeAtomicBoolValue()); + BoolValue *BoolVal2 = getHasValue(Env, Env.getValue(*Loc2)); + if (BoolVal2 == nullptr) + BoolVal2 = &Env.makeAtomicBoolValue(); - Env.setValue(*Loc1, *Val2); - Env.setValue(*Loc2, *Val1); + createOptionalValue(*Loc1, *BoolVal2, Env); + createOptionalValue(*Loc2, *BoolVal1, Env); } void transferSwapCall(const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { assert(E->getNumArgs() == 1); - transferSwap(maybeSkipPointer( - State.Env.getStorageLocation(*E->getImplicitObjectArgument(), - SkipPast::Reference), - State.Env), - State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference), - State.Env); + auto *OtherLoc = cast_or_null<AggregateStorageLocation>( + State.Env.getStorageLocationStrict(*E->getArg(0))); + transferSwap(getImplicitObjectLocation(*E, State.Env), OtherLoc, State.Env); } void transferStdSwapCall(const CallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { assert(E->getNumArgs() == 2); - transferSwap(State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference), - State.Env.getStorageLocation(*E->getArg(1), SkipPast::Reference), - State.Env); + auto *Arg0Loc = cast_or_null<AggregateStorageLocation>( + State.Env.getStorageLocationStrict(*E->getArg(0))); + auto *Arg1Loc = cast_or_null<AggregateStorageLocation>( + State.Env.getStorageLocationStrict(*E->getArg(1))); + transferSwap(Arg0Loc, Arg1Loc, State.Env); } void transferStdForwardCall(const CallExpr *E, const MatchFinder::MatchResult &, @@ -697,7 +692,7 @@ void transferStdForwardCall(const CallExpr *E, const MatchFinder::MatchResult &, Value *ValArg = State.Env.getValue(*LocArg); if (ValArg == nullptr) - ValArg = &createOptionalValue(State.Env, State.Env.makeAtomicBoolValue()); + ValArg = &createOptionalValue(State.Env.makeAtomicBoolValue(), State.Env); // Create a new storage location LocRet = &State.Env.createStorageLocation(*E); @@ -798,24 +793,24 @@ auto buildTransferMatchSwitch() { isOptionalInPlaceConstructor(), [](const CXXConstructExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { - assignOptionalValue(*E, State.Env, - State.Env.getBoolLiteralValue(true)); + constructOptionalValue(*E, State.Env, + State.Env.getBoolLiteralValue(true)); }) // nullopt_t::nullopt_t .CaseOfCFGStmt<CXXConstructExpr>( isNulloptConstructor(), [](const CXXConstructExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { - assignOptionalValue(*E, State.Env, - State.Env.getBoolLiteralValue(false)); + constructOptionalValue(*E, State.Env, + State.Env.getBoolLiteralValue(false)); }) // optional::optional(nullopt_t) .CaseOfCFGStmt<CXXConstructExpr>( isOptionalNulloptConstructor(), [](const CXXConstructExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { - assignOptionalValue(*E, State.Env, - State.Env.getBoolLiteralValue(false)); + constructOptionalValue(*E, State.Env, + State.Env.getBoolLiteralValue(false)); }) // optional::optional (value/conversion) .CaseOfCFGStmt<CXXConstructExpr>(isOptionalValueOrConversionConstructor(), @@ -867,8 +862,11 @@ auto buildTransferMatchSwitch() { isOptionalMemberCallWithName("emplace"), [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { - assignOptionalValue(*E->getImplicitObjectArgument(), State.Env, - State.Env.getBoolLiteralValue(true)); + if (AggregateStorageLocation *Loc = + getImplicitObjectLocation(*E, State.Env)) { + createOptionalValue(*Loc, State.Env.getBoolLiteralValue(true), + State.Env); + } }) // optional::reset @@ -876,8 +874,11 @@ auto buildTransferMatchSwitch() { isOptionalMemberCallWithName("reset"), [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { - assignOptionalValue(*E->getImplicitObjectArgument(), State.Env, - State.Env.getBoolLiteralValue(false)); + if (AggregateStorageLocation *Loc = + getImplicitObjectLocation(*E, State.Env)) { + createOptionalValue(*Loc, State.Env.getBoolLiteralValue(false), + State.Env); + } }) // optional::swap @@ -1043,7 +1044,7 @@ Value *UncheckedOptionalAccessModel::widen(QualType Type, Value &Prev, if (isa<TopBoolValue>(CurrentHasVal)) return &Current; } - return &createOptionalValue(CurrentEnv, CurrentEnv.makeTopBoolValue()); + return &createOptionalValue(CurrentEnv.makeTopBoolValue(), CurrentEnv); case ComparisonResult::Unknown: return nullptr; } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits