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 @@ -329,8 +329,8 @@ llvm::DenseSet &Visited, int Depth, int &CreatedValuesCount); - StorageLocation &skip(StorageLocation &Loc, SkipPast SP) const; - const StorageLocation &skip(const StorageLocation &Loc, SkipPast SP) const; + StorageLocation *skip(StorageLocation &Loc, SkipPast SP) const; + const StorageLocation *skip(const StorageLocation &Loc, SkipPast SP) const; // `DACtx` is not null and not owned by this object. DataflowAnalysisContext *DACtx; diff --git a/clang/include/clang/Analysis/FlowSensitive/Value.h b/clang/include/clang/Analysis/FlowSensitive/Value.h --- a/clang/include/clang/Analysis/FlowSensitive/Value.h +++ b/clang/include/clang/Analysis/FlowSensitive/Value.h @@ -186,17 +186,17 @@ /// Models a symbolic pointer. Specifically, any value of type `T*`. class PointerValue final : public Value { public: - explicit PointerValue(StorageLocation &PointeeLoc) + explicit PointerValue(StorageLocation *PointeeLoc) : Value(Kind::Pointer), PointeeLoc(PointeeLoc) {} static bool classof(const Value *Val) { return Val->getKind() == Kind::Pointer; } - StorageLocation &getPointerPointeeLoc() const { return PointeeLoc; } + StorageLocation *getPointerPointeeLoc() const { return PointeeLoc; } private: - StorageLocation &PointeeLoc; + StorageLocation *PointeeLoc; }; /// Models a value of `struct` or `class` type, with a flat map of fields to 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 @@ -58,7 +58,7 @@ } if (auto *IndVal1 = dyn_cast(Val1)) { auto *IndVal2 = cast(Val2); - return &IndVal1->getPointerPointeeLoc() == &IndVal2->getPointerPointeeLoc(); + return IndVal1->getPointerPointeeLoc() == IndVal2->getPointerPointeeLoc(); } return false; } @@ -349,7 +349,7 @@ StorageLocation *Environment::getStorageLocation(const ValueDecl &D, SkipPast SP) const { auto It = DeclToLoc.find(&D); - return It == DeclToLoc.end() ? nullptr : &skip(*It->second, SP); + return It == DeclToLoc.end() ? nullptr : skip(*It->second, SP); } void Environment::setStorageLocation(const Expr &E, StorageLocation &Loc) { @@ -362,7 +362,7 @@ SkipPast SP) const { // FIXME: Add a test with parens. auto It = ExprToLoc.find(&ignoreCFGOmittedNodes(E)); - return It == ExprToLoc.end() ? nullptr : &skip(*It->second, SP); + return It == ExprToLoc.end() ? nullptr : skip(*It->second, SP); } StorageLocation *Environment::getThisPointeeStorageLocation() const { @@ -486,7 +486,7 @@ setValue(PointeeLoc, *PointeeVal); } - return &takeOwnership(std::make_unique(PointeeLoc)); + return &takeOwnership(std::make_unique(&PointeeLoc)); } if (Type->isStructureOrClassType()) { @@ -515,26 +515,27 @@ return nullptr; } -StorageLocation &Environment::skip(StorageLocation &Loc, SkipPast SP) const { +StorageLocation *Environment::skip(StorageLocation &Loc, SkipPast SP) const { switch (SP) { case SkipPast::None: - return Loc; + return &Loc; case SkipPast::Reference: // References cannot be chained so we only need to skip past one level of // indirection. if (auto *Val = dyn_cast_or_null(getValue(Loc))) - return Val->getReferencePointeeLoc(); - return Loc; + return &Val->getReferencePointeeLoc(); + return &Loc; case SkipPast::ReferenceThenPointer: - StorageLocation &LocPastRef = skip(Loc, SkipPast::Reference); - if (auto *Val = dyn_cast_or_null(getValue(LocPastRef))) + StorageLocation *LocPastRef = skip(Loc, SkipPast::Reference); + assert(LocPastRef != nullptr); + if (auto *Val = dyn_cast_or_null(getValue(*LocPastRef))) return Val->getPointerPointeeLoc(); return LocPastRef; } llvm_unreachable("bad SkipPast kind"); } -const StorageLocation &Environment::skip(const StorageLocation &Loc, +const StorageLocation *Environment::skip(const StorageLocation &Loc, SkipPast SP) const { return skip(*const_cast(&Loc), SP); } 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 @@ -263,10 +263,14 @@ if (SubExprVal == nullptr) break; - auto &Loc = Env.createStorageLocation(*S); - Env.setStorageLocation(*S, Loc); - Env.setValue(Loc, Env.takeOwnership(std::make_unique( - SubExprVal->getPointerPointeeLoc()))); + // If PointeeLoc is null, then we are dereferencing a nullptr, skip + // creating a value for the dereference + if (auto *PointeeLoc = SubExprVal->getPointerPointeeLoc()) { + auto &Loc = Env.createStorageLocation(*S); + Env.setStorageLocation(*S, Loc); + Env.setValue(Loc, Env.takeOwnership(std::make_unique( + *SubExprVal->getPointerPointeeLoc()))); + } break; } case UO_AddrOf: { @@ -280,7 +284,7 @@ auto &PointerLoc = Env.createStorageLocation(*S); auto &PointerVal = - Env.takeOwnership(std::make_unique(*PointeeLoc)); + Env.takeOwnership(std::make_unique(PointeeLoc)); Env.setStorageLocation(*S, PointerLoc); Env.setValue(PointerLoc, PointerVal); break; @@ -311,7 +315,7 @@ auto &Loc = Env.createStorageLocation(*S); Env.setStorageLocation(*S, Loc); Env.setValue(Loc, Env.takeOwnership( - std::make_unique(*ThisPointeeLoc))); + std::make_unique(ThisPointeeLoc))); } void VisitMemberExpr(const MemberExpr *S) { 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 @@ -413,8 +413,9 @@ const auto *FooPtrVal = cast(BarPointeeVal->getChild(*FooPtrDecl)); - const StorageLocation &FooPtrPointeeLoc = FooPtrVal->getPointerPointeeLoc(); - EXPECT_THAT(Env.getValue(FooPtrPointeeLoc), IsNull()); + const StorageLocation *FooPtrPointeeLoc = FooPtrVal->getPointerPointeeLoc(); + ASSERT_THAT(FooPtrPointeeLoc, NotNull()); + EXPECT_THAT(Env.getValue(*FooPtrPointeeLoc), IsNull()); const auto *BazRefVal = cast(BarPointeeVal->getChild(*BazRefDecl)); @@ -424,8 +425,9 @@ const auto *BazPtrVal = cast(BarPointeeVal->getChild(*BazPtrDecl)); - const StorageLocation &BazPtrPointeeLoc = BazPtrVal->getPointerPointeeLoc(); - EXPECT_THAT(Env.getValue(BazPtrPointeeLoc), NotNull()); + const StorageLocation *BazPtrPointeeLoc = BazPtrVal->getPointerPointeeLoc(); + ASSERT_THAT(BazPtrPointeeLoc, NotNull()); + EXPECT_THAT(Env.getValue(*BazPtrPointeeLoc), NotNull()); }); } @@ -456,10 +458,10 @@ ASSERT_TRUE(isa_and_nonnull(FooLoc)); const PointerValue *FooVal = cast(Env.getValue(*FooLoc)); - const StorageLocation &FooPointeeLoc = FooVal->getPointerPointeeLoc(); - EXPECT_TRUE(isa(&FooPointeeLoc)); + const StorageLocation *FooPointeeLoc = FooVal->getPointerPointeeLoc(); + EXPECT_TRUE(isa_and_nonnull(FooPointeeLoc)); - const Value *FooPointeeVal = Env.getValue(FooPointeeLoc); + const Value *FooPointeeVal = Env.getValue(*FooPointeeLoc); EXPECT_TRUE(isa_and_nonnull(FooPointeeVal)); }); } @@ -488,106 +490,106 @@ // [[p]] } )"; - runDataflow( - Code, [](llvm::ArrayRef< - std::pair>> - Results, - ASTContext &ASTCtx) { - ASSERT_THAT(Results, ElementsAre(Pair("p", _))); - const Environment &Env = Results[0].second.Env; + runDataflow(Code, [](llvm::ArrayRef>> + 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()); + const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo"); + ASSERT_THAT(FooDecl, NotNull()); - ASSERT_TRUE(FooDecl->getType()->isPointerType()); - ASSERT_TRUE(FooDecl->getType() - ->getAs() - ->getPointeeType() - ->isStructureType()); - const auto FooFields = FooDecl->getType() - ->getAs() - ->getPointeeType() - ->getAsRecordDecl() - ->fields(); + ASSERT_TRUE(FooDecl->getType()->isPointerType()); + ASSERT_TRUE(FooDecl->getType() + ->getAs() + ->getPointeeType() + ->isStructureType()); + const auto FooFields = FooDecl->getType() + ->getAs() + ->getPointeeType() + ->getAsRecordDecl() + ->fields(); - FieldDecl *BarDecl = nullptr; - for (FieldDecl *Field : FooFields) { - if (Field->getNameAsString() == "Bar") { - BarDecl = Field; - } else { - FAIL() << "Unexpected field: " << Field->getNameAsString(); - } - } - ASSERT_THAT(BarDecl, NotNull()); + FieldDecl *BarDecl = nullptr; + for (FieldDecl *Field : FooFields) { + if (Field->getNameAsString() == "Bar") { + BarDecl = Field; + } else { + FAIL() << "Unexpected field: " << Field->getNameAsString(); + } + } + ASSERT_THAT(BarDecl, NotNull()); - ASSERT_TRUE(BarDecl->getType()->isPointerType()); - ASSERT_TRUE(BarDecl->getType() - ->getAs() - ->getPointeeType() - ->isStructureType()); - const auto BarFields = BarDecl->getType() - ->getAs() - ->getPointeeType() - ->getAsRecordDecl() - ->fields(); - - FieldDecl *FooRefDecl = nullptr; - FieldDecl *FooPtrDecl = nullptr; - FieldDecl *BazRefDecl = nullptr; - FieldDecl *BazPtrDecl = nullptr; - for (FieldDecl *Field : BarFields) { - if (Field->getNameAsString() == "FooRef") { - FooRefDecl = Field; - } else if (Field->getNameAsString() == "FooPtr") { - FooPtrDecl = Field; - } else if (Field->getNameAsString() == "BazRef") { - BazRefDecl = Field; - } else if (Field->getNameAsString() == "BazPtr") { - BazPtrDecl = Field; - } else { - FAIL() << "Unexpected field: " << Field->getNameAsString(); - } - } - ASSERT_THAT(FooRefDecl, NotNull()); - ASSERT_THAT(FooPtrDecl, NotNull()); - ASSERT_THAT(BazRefDecl, NotNull()); - ASSERT_THAT(BazPtrDecl, NotNull()); + ASSERT_TRUE(BarDecl->getType()->isPointerType()); + ASSERT_TRUE(BarDecl->getType() + ->getAs() + ->getPointeeType() + ->isStructureType()); + const auto BarFields = BarDecl->getType() + ->getAs() + ->getPointeeType() + ->getAsRecordDecl() + ->fields(); - const auto *FooLoc = cast( - Env.getStorageLocation(*FooDecl, SkipPast::None)); - const auto *FooVal = cast(Env.getValue(*FooLoc)); - const auto *FooPointeeVal = - cast(Env.getValue(FooVal->getPointerPointeeLoc())); + FieldDecl *FooRefDecl = nullptr; + FieldDecl *FooPtrDecl = nullptr; + FieldDecl *BazRefDecl = nullptr; + FieldDecl *BazPtrDecl = nullptr; + for (FieldDecl *Field : BarFields) { + if (Field->getNameAsString() == "FooRef") { + FooRefDecl = Field; + } else if (Field->getNameAsString() == "FooPtr") { + FooPtrDecl = Field; + } else if (Field->getNameAsString() == "BazRef") { + BazRefDecl = Field; + } else if (Field->getNameAsString() == "BazPtr") { + BazPtrDecl = Field; + } else { + FAIL() << "Unexpected field: " << Field->getNameAsString(); + } + } + ASSERT_THAT(FooRefDecl, NotNull()); + ASSERT_THAT(FooPtrDecl, NotNull()); + ASSERT_THAT(BazRefDecl, NotNull()); + ASSERT_THAT(BazPtrDecl, NotNull()); - const auto *BarVal = - cast(FooPointeeVal->getChild(*BarDecl)); - const auto *BarPointeeVal = - cast(Env.getValue(BarVal->getPointerPointeeLoc())); - - const auto *FooRefVal = - cast(BarPointeeVal->getChild(*FooRefDecl)); - const StorageLocation &FooRefPointeeLoc = - FooRefVal->getReferencePointeeLoc(); - EXPECT_THAT(Env.getValue(FooRefPointeeLoc), IsNull()); - - const auto *FooPtrVal = - cast(BarPointeeVal->getChild(*FooPtrDecl)); - const StorageLocation &FooPtrPointeeLoc = - FooPtrVal->getPointerPointeeLoc(); - EXPECT_THAT(Env.getValue(FooPtrPointeeLoc), IsNull()); - - const auto *BazRefVal = - cast(BarPointeeVal->getChild(*BazRefDecl)); - const StorageLocation &BazRefPointeeLoc = - BazRefVal->getReferencePointeeLoc(); - EXPECT_THAT(Env.getValue(BazRefPointeeLoc), NotNull()); - - const auto *BazPtrVal = - cast(BarPointeeVal->getChild(*BazPtrDecl)); - const StorageLocation &BazPtrPointeeLoc = - BazPtrVal->getPointerPointeeLoc(); - EXPECT_THAT(Env.getValue(BazPtrPointeeLoc), NotNull()); - }); + const auto *FooLoc = cast( + Env.getStorageLocation(*FooDecl, SkipPast::None)); + const auto *FooVal = cast(Env.getValue(*FooLoc)); + const auto *FooPointeeLoc = FooVal->getPointerPointeeLoc(); + ASSERT_THAT(FooPointeeLoc, NotNull()); + const auto *FooPointeeVal = cast(Env.getValue(*FooPointeeLoc)); + + const auto *BarVal = cast(FooPointeeVal->getChild(*BarDecl)); + const auto *BarPointeeLoc = BarVal->getPointerPointeeLoc(); + ASSERT_THAT(BarPointeeLoc, NotNull()); + const auto *BarPointeeVal = cast(Env.getValue(*BarPointeeLoc)); + + const auto *FooRefVal = + cast(BarPointeeVal->getChild(*FooRefDecl)); + const StorageLocation &FooRefPointeeLoc = + FooRefVal->getReferencePointeeLoc(); + EXPECT_THAT(Env.getValue(FooRefPointeeLoc), IsNull()); + + const auto *FooPtrVal = + cast(BarPointeeVal->getChild(*FooPtrDecl)); + const StorageLocation *FooPtrPointeeLoc = FooPtrVal->getPointerPointeeLoc(); + ASSERT_THAT(FooPtrPointeeLoc, NotNull()); + EXPECT_THAT(Env.getValue(*FooPtrPointeeLoc), IsNull()); + + const auto *BazRefVal = + cast(BarPointeeVal->getChild(*BazRefDecl)); + const StorageLocation &BazRefPointeeLoc = + BazRefVal->getReferencePointeeLoc(); + EXPECT_THAT(Env.getValue(BazRefPointeeLoc), NotNull()); + + const auto *BazPtrVal = + cast(BarPointeeVal->getChild(*BazPtrDecl)); + const StorageLocation *BazPtrPointeeLoc = BazPtrVal->getPointerPointeeLoc(); + ASSERT_THAT(BazPtrPointeeLoc, NotNull()); + EXPECT_THAT(Env.getValue(*BazPtrPointeeLoc), NotNull()); + }); } TEST_F(TransferTest, MultipleVarsDecl) { @@ -805,7 +807,9 @@ const auto *BarVal = cast(Env.getValue(*BarDecl, SkipPast::None)); - EXPECT_EQ(Env.getValue(BarVal->getPointerPointeeLoc()), FooVal); + const auto *BarPointeeLoc = BarVal->getPointerPointeeLoc(); + ASSERT_THAT(BarPointeeLoc, NotNull()); + EXPECT_EQ(Env.getValue(*BarPointeeLoc), FooVal); const ValueDecl *BazDecl = findValueDecl(ASTCtx, "Baz"); ASSERT_THAT(BazDecl, NotNull()); @@ -1010,10 +1014,10 @@ ASSERT_TRUE(isa_and_nonnull(FooLoc)); const PointerValue *FooVal = cast(Env.getValue(*FooLoc)); - const StorageLocation &FooPointeeLoc = FooVal->getPointerPointeeLoc(); - EXPECT_TRUE(isa(&FooPointeeLoc)); + const StorageLocation *FooPointeeLoc = FooVal->getPointerPointeeLoc(); + EXPECT_TRUE(isa_and_nonnull(FooPointeeLoc)); - const Value *FooPointeeVal = Env.getValue(FooPointeeLoc); + const Value *FooPointeeVal = Env.getValue(*FooPointeeLoc); EXPECT_TRUE(isa_and_nonnull(FooPointeeVal)); }); } @@ -2247,7 +2251,7 @@ Env.getStorageLocation(*FooDecl, SkipPast::None)); const auto *BarVal = cast(Env.getValue(*BarDecl, SkipPast::None)); - EXPECT_EQ(&BarVal->getPointerPointeeLoc(), FooLoc); + EXPECT_EQ(BarVal->getPointerPointeeLoc(), FooLoc); }); } @@ -2276,8 +2280,8 @@ cast(Env.getValue(*FooDecl, SkipPast::None)); const auto *BarVal = cast(Env.getValue(*BarDecl, SkipPast::None)); - EXPECT_EQ(&BarVal->getPointerPointeeLoc(), - &FooVal->getPointerPointeeLoc()); + EXPECT_EQ(BarVal->getPointerPointeeLoc(), + FooVal->getPointerPointeeLoc()); }); } @@ -2308,7 +2312,7 @@ const auto *BarVal = cast(Env.getValue(*BarDecl, SkipPast::None)); EXPECT_EQ(&BarVal->getReferencePointeeLoc(), - &FooVal->getPointerPointeeLoc()); + FooVal->getPointerPointeeLoc()); }); } @@ -2362,31 +2366,33 @@ /*[[p]]*/ } )"; - runDataflow( - Code, [](llvm::ArrayRef< - std::pair>> - Results, - ASTContext &ASTCtx) { - ASSERT_THAT(Results, ElementsAre(Pair("p", _))); - const Environment &Env = Results[0].second.Env; + 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()); + const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo"); + ASSERT_THAT(FooDecl, NotNull()); - const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar"); - ASSERT_THAT(BarDecl, NotNull()); + const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar"); + ASSERT_THAT(BarDecl, NotNull()); - const auto *FooVal = - cast(Env.getValue(*FooDecl, SkipPast::None)); - const auto *FooPointeeVal = - cast(Env.getValue(FooVal->getPointerPointeeLoc())); + const auto *FooVal = + cast(Env.getValue(*FooDecl, SkipPast::None)); + const auto *FooPointeeLoc = FooVal->getPointerPointeeLoc(); + ASSERT_THAT(FooPointeeLoc, NotNull()); + const auto *FooPointeeVal = + cast(Env.getValue(*FooPointeeLoc)); - const auto *BarVal = dyn_cast_or_null( - Env.getValue(*BarDecl, SkipPast::None)); - ASSERT_THAT(BarVal, NotNull()); + const auto *BarVal = dyn_cast_or_null( + Env.getValue(*BarDecl, SkipPast::None)); + ASSERT_THAT(BarVal, NotNull()); - EXPECT_EQ(BarVal, FooPointeeVal); - }); + EXPECT_EQ(BarVal, FooPointeeVal); + }); } TEST_F(TransferTest, AggregateInitialization) { @@ -3340,7 +3346,7 @@ dyn_cast(InnerEnv.getValue(*LDecl, SkipPast::None)); ASSERT_THAT(LVal, NotNull()); - EXPECT_EQ(&LVal->getPointerPointeeLoc(), + EXPECT_EQ(LVal->getPointerPointeeLoc(), InnerEnv.getStorageLocation(*ValDecl, SkipPast::Reference)); // Outer. @@ -3350,7 +3356,7 @@ // The loop body may not have been executed, so we should not conclude // that `l` points to `val`. - EXPECT_NE(&LVal->getPointerPointeeLoc(), + EXPECT_NE(LVal->getPointerPointeeLoc(), OuterEnv.getStorageLocation(*ValDecl, SkipPast::Reference)); }); } 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 @@ -676,7 +676,7 @@ Env.getStorageLocation(*FooDecl, SkipPast::None)); const auto *BarVal = cast(Env.getValue(*BarDecl, SkipPast::None)); - EXPECT_EQ(&BarVal->getPointerPointeeLoc(), FooLoc); + EXPECT_EQ(BarVal->getPointerPointeeLoc(), FooLoc); }); }