diff --git a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h --- a/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h +++ b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h @@ -324,6 +324,7 @@ llvm::DenseMap DeclToLoc; llvm::DenseMap ExprToLoc; + // FIXME: Move this into `Environment`. StorageLocation *ThisPointeeLoc = nullptr; // Null pointer values, keyed by the canonical pointee type. 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 @@ -222,6 +222,9 @@ /// in the environment. StorageLocation *getThisPointeeStorageLocation() const; + /// Returns the storage location of the return value or null, if unset. + StorageLocation *getReturnStorageLocation() const; + /// Returns a pointer value that represents a null pointer. Calls with /// `PointeeType` that are canonically equivalent will return the same result. PointerValue &getOrCreateNullPointerValue(QualType PointeeType); @@ -374,6 +377,11 @@ // `DACtx` is not null and not owned by this object. DataflowAnalysisContext *DACtx; + // In a properly initialized `Environment`, `ReturnLoc` should only be null if + // its `DeclContext` could not be cast to a `FunctionDecl`. + StorageLocation *ReturnLoc = nullptr; + // FIXME: Move `ThisPointeeLoc` here from `DataflowAnalysisContext`. + // Maps from program declarations and statements to storage locations that are // assigned to them. Unlike the maps in `DataflowAnalysisContext`, these // include only storage locations that are in scope for a particular basic 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 @@ -154,9 +154,9 @@ : DACtx(&DACtx), FlowConditionToken(&DACtx.makeFlowConditionToken()) {} Environment::Environment(const Environment &Other) - : DACtx(Other.DACtx), DeclToLoc(Other.DeclToLoc), - ExprToLoc(Other.ExprToLoc), LocToVal(Other.LocToVal), - MemberLocToStruct(Other.MemberLocToStruct), + : DACtx(Other.DACtx), ReturnLoc(Other.ReturnLoc), + DeclToLoc(Other.DeclToLoc), ExprToLoc(Other.ExprToLoc), + LocToVal(Other.LocToVal), MemberLocToStruct(Other.MemberLocToStruct), FlowConditionToken(&DACtx->forkFlowCondition(*Other.FlowConditionToken)) { } @@ -179,6 +179,9 @@ if (Value *ParamVal = createValue(ParamDecl->getType())) setValue(ParamLoc, *ParamVal); } + + QualType ReturnType = FuncDecl->getReturnType(); + ReturnLoc = &createStorageLocation(ReturnType); } if (const auto *MethodDecl = dyn_cast(&DeclCtx)) { @@ -209,6 +212,9 @@ // FIXME: In order to allow the callee to reference globals, we probably need // to call `initGlobalVars` here in some way. + QualType ReturnType = FuncDecl->getReturnType(); + Env.ReturnLoc = &Env.createStorageLocation(ReturnType); + auto ParamIt = FuncDecl->param_begin(); auto ArgIt = Call->arg_begin(); auto ArgEnd = Call->arg_end(); @@ -241,12 +247,13 @@ } void Environment::popCall(const Environment &CalleeEnv) { - // We ignore `DACtx` because it's already the same in both. We don't bring - // back `DeclToLoc` and `ExprToLoc` because we want to be able to later - // analyze the same callee in a different context, and `setStorageLocation` - // requires there to not already be a storage location assigned. Conceptually, - // these maps capture information from the local scope, so when popping that - // scope, we do not propagate the maps. + // We ignore `DACtx` because it's already the same in both. We don't want the + // callee's `ReturnLoc`. We don't bring back `DeclToLoc` and `ExprToLoc` + // because we want to be able to later analyze the same callee in a different + // context, and `setStorageLocation` requires there to not already be a + // storage location assigned. Conceptually, these maps capture information + // from the local scope, so when popping that scope, we do not propagate the + // maps. this->LocToVal = std::move(CalleeEnv.LocToVal); this->MemberLocToStruct = std::move(CalleeEnv.MemberLocToStruct); this->FlowConditionToken = std::move(CalleeEnv.FlowConditionToken); @@ -256,6 +263,9 @@ Environment::ValueModel &Model) const { assert(DACtx == Other.DACtx); + if (ReturnLoc != Other.ReturnLoc) + return false; + if (DeclToLoc != Other.DeclToLoc) return false; @@ -285,11 +295,14 @@ LatticeJoinEffect Environment::join(const Environment &Other, Environment::ValueModel &Model) { assert(DACtx == Other.DACtx); + assert(ReturnLoc == Other.ReturnLoc); auto Effect = LatticeJoinEffect::Unchanged; Environment JoinedEnv(*DACtx); + JoinedEnv.ReturnLoc = ReturnLoc; + JoinedEnv.DeclToLoc = intersectDenseMaps(DeclToLoc, Other.DeclToLoc); if (DeclToLoc.size() != JoinedEnv.DeclToLoc.size()) Effect = LatticeJoinEffect::Changed; @@ -382,6 +395,10 @@ return DACtx->getThisPointeeStorageLocation(); } +StorageLocation *Environment::getReturnStorageLocation() const { + return ReturnLoc; +} + PointerValue &Environment::getOrCreateNullPointerValue(QualType PointeeType) { return DACtx->getOrCreateNullPointerValue(PointeeType); } 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 @@ -332,6 +332,25 @@ std::make_unique(*ThisPointeeLoc))); } + void VisitReturnStmt(const ReturnStmt *S) { + auto *Ret = S->getRetValue(); + if (Ret == nullptr) + return; + + auto *Val = Env.getValue(*Ret, SkipPast::None); + if (Val == nullptr) + return; + + // FIXME: Support reference-type returns. + if (Val->getKind() == Value::Kind::Reference) + return; + + auto *Loc = Env.getReturnStorageLocation(); + assert(Loc != nullptr); + // FIXME: Model NRVO. + Env.setValue(*Loc, *Val); + } + void VisitMemberExpr(const MemberExpr *S) { ValueDecl *Member = S->getMemberDecl(); assert(Member != nullptr); @@ -540,7 +559,11 @@ auto ExitState = (*BlockToOutputState)[ExitBlock]; assert(ExitState); - Env.popCall(ExitState->Env); + auto &ExitEnv = ExitState->Env; + auto *ReturnLoc = ExitEnv.getReturnStorageLocation(); + assert(ReturnLoc != nullptr); + Env.setStorageLocation(*S, *ReturnLoc); + Env.popCall(ExitEnv); } } diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -4121,4 +4121,112 @@ /*.BuiltinTransferOptions=*/{/*.ContextSensitive=*/true}}); } +TEST(TransferTest, ContextSensitiveReturnVoid) { + std::string Code = R"( + void Noop() { return; } + + void target() { + Noop(); + // [[p]] + } + )"; + runDataflow(Code, + [](llvm::ArrayRef< + std::pair>> + Results, + ASTContext &ASTCtx) { + ASSERT_THAT(Results, ElementsAre(Pair("p", _))); + // This just tests that the analysis doesn't crash. + }, + {/*.ApplyBuiltinTransfer=*/true, + /*.BuiltinTransferOptions=*/{/*.ContextSensitive=*/true}}); +} + +TEST(TransferTest, ContextSensitiveReturnTrue) { + std::string Code = R"( + bool GiveBool() { return true; } + + void target() { + bool Foo = GiveBool(); + // [[p]] + } + )"; + runDataflow(Code, + [](llvm::ArrayRef< + std::pair>> + Results, + ASTContext &ASTCtx) { + ASSERT_THAT(Results, ElementsAre(Pair("p", _))); + const Environment &Env = Results[0].second.Env; + + const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo"); + ASSERT_THAT(FooDecl, NotNull()); + + auto &FooVal = + *cast(Env.getValue(*FooDecl, SkipPast::None)); + EXPECT_TRUE(Env.flowConditionImplies(FooVal)); + }, + {/*.ApplyBuiltinTransfer=*/true, + /*.BuiltinTransferOptions=*/{/*.ContextSensitive=*/true}}); +} + +TEST(TransferTest, ContextSensitiveReturnFalse) { + std::string Code = R"( + bool GiveBool() { return false; } + + void target() { + bool Foo = GiveBool(); + // [[p]] + } + )"; + runDataflow(Code, + [](llvm::ArrayRef< + std::pair>> + Results, + ASTContext &ASTCtx) { + ASSERT_THAT(Results, ElementsAre(Pair("p", _))); + const Environment &Env = Results[0].second.Env; + + const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo"); + ASSERT_THAT(FooDecl, NotNull()); + + auto &FooVal = + *cast(Env.getValue(*FooDecl, SkipPast::None)); + EXPECT_TRUE(Env.flowConditionImplies(Env.makeNot(FooVal))); + }, + {/*.ApplyBuiltinTransfer=*/true, + /*.BuiltinTransferOptions=*/{/*.ContextSensitive=*/true}}); +} + +TEST(TransferTest, ContextSensitiveReturnArg) { + std::string Code = R"( + bool GiveBool(); + bool GiveBack(bool Arg) { return Arg; } + + void target() { + bool Foo = GiveBool(); + bool Bar = GiveBack(Foo); + bool Baz = Foo == Bar; + // [[p]] + } + )"; + runDataflow(Code, + [](llvm::ArrayRef< + std::pair>> + Results, + ASTContext &ASTCtx) { + ASSERT_THAT(Results, ElementsAre(Pair("p", _))); + const Environment &Env = Results[0].second.Env; + + const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz"); + ASSERT_THAT(BazDecl, NotNull()); + + auto &BazVal = + *cast(Env.getValue(*BazDecl, SkipPast::None)); + EXPECT_TRUE(Env.flowConditionImplies(BazVal)); + }, + {/*.ApplyBuiltinTransfer=*/true, + /*.BuiltinTransferOptions=*/{/*.ContextSensitive=*/true}}); +} + } // namespace