Author: Yitzhak Mandelbaum Date: 2023-01-10T15:48:00Z New Revision: 3ce03c42dbb46531968c527d80c0243c2db7bc0e
URL: https://github.com/llvm/llvm-project/commit/3ce03c42dbb46531968c527d80c0243c2db7bc0e DIFF: https://github.com/llvm/llvm-project/commit/3ce03c42dbb46531968c527d80c0243c2db7bc0e.diff LOG: [clang][dataflow] Fix 2 bugs in `MemberExpr` interpretation. There were two (small) bugs causing crashes in the analysis. This patch fixes both of them. 1. An enum value was accessed as a class member. Now, the engine gracefully ignores such member expressions. 2. Field access in `MemberExpr` of struct/class-typed global variables. Analysis didn't interpret fields of global vars, because the vars were initialized before the fields were added to the "allowlist". Now, the allowlist is set _before_ init of globals. Differential Revision: https://reviews.llvm.org/D141384 Added: Modified: clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp clang/lib/Analysis/FlowSensitive/Transfer.cpp clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp clang/unittests/Analysis/FlowSensitive/TransferTest.cpp Removed: ################################################################################ diff --git a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp index 7d10a75b36e3e..37d200509e8d2 100644 --- a/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp +++ b/clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp @@ -250,11 +250,12 @@ Environment::Environment(DataflowAnalysisContext &DACtx, } getFieldsAndGlobalVars(*FuncDecl->getBody(), Fields, Vars); - initVars(Vars); - // These have to be set before the lines that follow to ensure that create* - // work correctly for structs. + // These have to be added before the lines that follow to ensure that + // `create*` work correctly for structs. DACtx.addModeledFields(Fields); + initVars(Vars); + for (const auto *ParamDecl : FuncDecl->parameters()) { assert(ParamDecl != nullptr); auto &ParamLoc = createStorageLocation(*ParamDecl); @@ -341,9 +342,13 @@ void Environment::pushCallInternal(const FunctionDecl *FuncDecl, } } getFieldsAndGlobalVars(*FuncDecl->getBody(), Fields, Vars); - initVars(Vars); + + // These have to be added before the lines that follow to ensure that + // `create*` work correctly for structs. DACtx->addModeledFields(Fields); + initVars(Vars); + const auto *ParamIt = FuncDecl->param_begin(); // FIXME: Parameters don't always map to arguments 1:1; examples include diff --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp index c2bf18754be51..259b82d647981 100644 --- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp +++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp @@ -461,6 +461,10 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> { if (Member->isFunctionOrFunctionTemplate()) return; + // FIXME: if/when we add support for modeling enums, use that support here. + if (isa<EnumConstantDecl>(Member)) + return; + if (auto *D = dyn_cast<VarDecl>(Member)) { if (D->hasGlobalStorage()) { auto *VarDeclLoc = Env.getStorageLocation(*D, SkipPast::None); diff --git a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp index e66ed70be4feb..dfbd4ff274154 100644 --- a/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/DataflowEnvironmentTest.cpp @@ -118,7 +118,7 @@ TEST_F(EnvironmentTest, InitGlobalVarsFun) { Context); const auto *Fun = selectFirst<FunctionDecl>("target", Results); const auto *Var = selectFirst<VarDecl>("global", Results); - ASSERT_TRUE(Fun != nullptr); + ASSERT_THAT(Fun, NotNull()); ASSERT_THAT(Var, NotNull()); // Verify the global variable is populated when we analyze `Target`. @@ -126,6 +126,53 @@ TEST_F(EnvironmentTest, InitGlobalVarsFun) { EXPECT_THAT(Env.getValue(*Var, SkipPast::None), NotNull()); } +TEST_F(EnvironmentTest, InitGlobalVarsFieldFun) { + using namespace ast_matchers; + + std::string Code = R"cc( + struct S { int Bar; }; + S Global = {0}; + int Target () { return Global.Bar; } + )cc"; + + auto Unit = + tooling::buildASTFromCodeWithArgs(Code, {"-fsyntax-only", "-std=c++11"}); + auto &Context = Unit->getASTContext(); + + ASSERT_EQ(Context.getDiagnostics().getClient()->getNumErrors(), 0U); + + auto Results = + match(decl(anyOf(varDecl(hasName("Global")).bind("global"), + functionDecl(hasName("Target")).bind("target"))), + Context); + const auto *Fun = selectFirst<FunctionDecl>("target", Results); + const auto *GlobalDecl = selectFirst<VarDecl>("global", Results); + ASSERT_THAT(Fun, NotNull()); + ASSERT_THAT(GlobalDecl, NotNull()); + + ASSERT_TRUE(GlobalDecl->getType()->isStructureType()); + auto GlobalFields = GlobalDecl->getType()->getAsRecordDecl()->fields(); + + FieldDecl *BarDecl = nullptr; + for (FieldDecl *Field : GlobalFields) { + if (Field->getNameAsString() == "Bar") { + BarDecl = Field; + break; + } + FAIL() << "Unexpected field: " << Field->getNameAsString(); + } + ASSERT_THAT(BarDecl, NotNull()); + + // Verify the global variable is populated when we analyze `Target`. + Environment Env(DAContext, *Fun); + const auto *GlobalLoc = cast<AggregateStorageLocation>( + Env.getStorageLocation(*GlobalDecl, SkipPast::None)); + const auto *GlobalVal = cast<StructValue>(Env.getValue(*GlobalLoc)); + const auto *BarVal = GlobalVal->getChild(*BarDecl); + ASSERT_THAT(BarVal, NotNull()); + EXPECT_TRUE(isa<IntegerValue>(BarVal)); +} + TEST_F(EnvironmentTest, InitGlobalVarsConstructor) { using namespace ast_matchers; diff --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp index 6c6584cdc8921..b4bf699b22918 100644 --- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp @@ -1066,6 +1066,28 @@ TEST(TransferTest, StructMember) { }); } +TEST(TransferTest, StructMemberEnum) { + std::string Code = R"( + struct A { + int Bar; + enum E { ONE, TWO }; + }; + + void target(A Foo) { + A::E Baz = Foo.ONE; + // [[p]] + } + )"; + // Minimal expectations -- we're just testing that it doesn't crash, since + // enums aren't interpreted. + runDataflow( + Code, + [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results, + ASTContext &ASTCtx) { + EXPECT_THAT(Results.keys(), UnorderedElementsAre("p")); + }); +} + TEST(TransferTest, DerivedBaseMemberClass) { std::string Code = R"( class A { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
