[clang][dataflow] Don't analyze templated declarations.

[mboehme]

Summary

Attempting to analyze templated code doesn't have a good cost-benefit ratio. We
have so far done a best-effort attempt at this, but maintaining this support has
an ongoing high maintenance cost because the AST for templates can violate a lot
of the invariants that otherwise hold for the AST of concrete code. As just one
example, in concrete code the operand of a UnaryOperator '*' is always a prvalue
(https://godbolt.org/z/s3e5xxMd1), but in templates this isn't true
(https://godbolt.org/z/6W9xxGvoM).

Further rationale for not analyzing templates:

• The semantics of a template itself are weakly defined; semantics can depend strongly on the concrete template arguments. Analyzing the template itself (as opposed to an instantiation) therefore has limited value.

• Analyzing templates requires a lot of special-case code that isn't necessary for concrete code because dependent types are hard to deal with and the AST violates invariants that otherwise hold for concrete code (see above).

• There's precedent in that neither Clang Static Analyzer nor the flow-sensitive warnings in Clang (such as uninitialized variables) support analyzing templates.

[gribozavr2]

clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h

36

Maybe this is too much for a drive-by fix, but since you are changing this API anyway, I have to ask - now that D is nonnull, isn't S strictly redundant? Isn't D always a FunctionDecl, and S its body?

[gribozavr2]

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

2537

This DerefDependentPtr test was originally added in https://reviews.llvm.org/D117567 and re-landed in https://github.com/llvm/llvm-project/commit/acd4b0359097dd8ad166d569a4566879e82a2793

Could you try to revert the effects of those commits? Specifically:

• remove reference skipping from the transfer function for UO_Deref (and the comment),

• remove the -fno-delayed-template-parsing flag from clang/unittests/Analysis/FlowSensitive/TransferTest.cpp.

[mboehme]

Changes in response to review comments

[mboehme]

clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h

36

Isn't D always a FunctionDecl, and S its body?

This is true for all of the callsites that I've seen, but I think I remember talking to someone who said the intent was that you might also use this to analyze the initializer of a global variable (where D would be the variable declaration and S would be the initializer). It seems telling, at least, that the interface expects only a Decl, not a FunctionDecl.

So I'm hesitant to remove this API entirely without more investigation. It certainly makes sense though to add an overload that takes a FunctionDecl and no Stmt, which will cover the vast majority of use cases. I'd like to do that in a separate patch though.

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

2537

Thanks for pointing this out! Done.

[mboehme]

Eliminate warning for use of deprecated function

[mboehme]

Eliminate warnings for calling deprecated functions

[mboehme]

Set -fnodelayed-template-parsing again.

This is needed to make tests pass on Windows.

[mboehme]

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

2537

• remove the -fno-delayed-template-parsing flag from clang/unittests/Analysis/FlowSensitive/TransferTest.cpp.

I've re-added this because removing it causes tests to fail on Windows.

-fnodelayed-template-parsing is the default everywhere except on Windows, so setting it explicitly makes tests on Windows behave the same as on other platforms. I've added a comment explaining this.

The reason that -fnodelayed-template-parsing was causing the tests to fail is that checkDataflow() only looks as function declarations that have a body, and late-parsed templated functions don't have bodies.

[mboehme]

Spell -fno-delayed-template-parsing correctly (it was missing a hyphen).

[uabelho]

Hello,

../../clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:43:27: error: 'build' is deprecated: Use the version that takes a const Decl & instead [-Werror,-Wdeprecated-declarations]
      ControlFlowContext::build(&FuncDecl, *FuncDecl.getBody(), ASTCtx);
                          ^
../../clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h:41:3: note: 'build' has been explicitly marked deprecated here
  LLVM_DEPRECATED("Use the version that takes a const Decl & instead", "")
  ^
../include/llvm/Support/Compiler.h:143:50: note: expanded from macro 'LLVM_DEPRECATED'
#define LLVM_DEPRECATED(MSG, FIX) __attribute__((deprecated(MSG, FIX)))
                                                 ^
1 error generated.

[mboehme]

In D150352#4342480, @uabelho wrote:

Hello,

../../clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:43:27: error: 'build' is deprecated: Use the version that takes a const Decl & instead [-Werror,-Wdeprecated-declarations]
      ControlFlowContext::build(&FuncDecl, *FuncDecl.getBody(), ASTCtx);
                          ^
../../clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h:41:3: note: 'build' has been explicitly marked deprecated here
  LLVM_DEPRECATED("Use the version that takes a const Decl & instead", "")
  ^
../include/llvm/Support/Compiler.h:143:50: note: expanded from macro 'LLVM_DEPRECATED'
#define LLVM_DEPRECATED(MSG, FIX) __attribute__((deprecated(MSG, FIX)))
                                                 ^
1 error generated.

Sorry for the breakage. I ran check-clang and fixed all the deprecation warnings, so I'm not sure how I missed this one.

Thank you to @amyk for the fix.

clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h

 namespace clang {
 namespace dataflow {
 
 /// Holds CFG and other derived context that is needed to perform dataflow
 /// analysis.
 class ControlFlowContext {
 public:
   /// Builds a ControlFlowContext from an AST node. `D` is the function in which
-  /// `S` resides and must not be null.
+  /// `S` resides. `D.isTemplated()` must be false.
+  static llvm::Expected<ControlFlowContext> build(const Decl &D, Stmt &S,
+                                                  ASTContext &C);
+
+  /// Builds a ControlFlowContext from an AST node. `D` is the function in which
+  /// `S` resides. `D` must not be null and `D->isTemplated()` must be false.
+  LLVM_DEPRECATED("Use the version that takes a const Decl & instead", "")
   static llvm::Expected<ControlFlowContext> build(const Decl *D, Stmt &S,
                                                   ASTContext &C);
 
   /// Returns the `Decl` containing the statement used to construct the CFG, if
   /// available.
   const Decl *getDecl() const { return ContainingDecl; }
 
   /// Returns the CFG that is stored in this context.

clang/lib/Analysis/FlowSensitive/ControlFlowContext.cpp

for (const CFGBlock *Succ : Block->succs())
       if (Succ)
         BlocksToVisit.push_back(Succ);
   }
 
   return BlockReachable;
 }
 
 llvm::Expected<ControlFlowContext>
-ControlFlowContext::build(const Decl *D, Stmt &S, ASTContext &C) {
+ControlFlowContext::build(const Decl &D, Stmt &S, ASTContext &C) {
+  if (D.isTemplated())
+    return llvm::createStringError(
+        std::make_error_code(std::errc::invalid_argument),
+        "Cannot analyze templated declarations");
+
   CFG::BuildOptions Options;
   Options.PruneTriviallyFalseEdges = true;
   Options.AddImplicitDtors = true;
   Options.AddTemporaryDtors = true;
   Options.AddInitializers = true;
   Options.AddCXXDefaultInitExprInCtors = true;
 
   // Ensure that all sub-expressions in basic blocks are evaluated.
   Options.setAllAlwaysAdd();
 
-  auto Cfg = CFG::buildCFG(D, &S, &C, Options);
+  auto Cfg = CFG::buildCFG(&D, &S, &C, Options);
   if (Cfg == nullptr)
     return llvm::createStringError(
         std::make_error_code(std::errc::invalid_argument),
         "CFG::buildCFG failed");
 
   llvm::DenseMap<const Stmt *, const CFGBlock *> StmtToBlock =
       buildStmtToBasicBlockMap(*Cfg);
 
   llvm::BitVector BlockReachable = findReachableBlocks(*Cfg);
 
-  return ControlFlowContext(D, std::move(Cfg), std::move(StmtToBlock),
+  return ControlFlowContext(&D, std::move(Cfg), std::move(StmtToBlock),
                             std::move(BlockReachable));
 }
 
+llvm::Expected<ControlFlowContext>
+ControlFlowContext::build(const Decl *D, Stmt &S, ASTContext &C) {
+  if (D == nullptr)
+    return llvm::createStringError(
+        std::make_error_code(std::errc::invalid_argument),
+        "Declaration must not be null");
+
+  return build(*D, S, C);
+}
+
 } // namespace dataflow
 } // namespace clang

clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp

DataflowAnalysisContext::getControlFlowContext(const FunctionDecl *F) {
   F = F->getDefinition();
   if (F == nullptr)
     return nullptr;
   auto It = FunctionContexts.find(F);
   if (It != FunctionContexts.end())
     return &It->second;
 
   if (Stmt *Body = F->getBody()) {
-    auto CFCtx = ControlFlowContext::build(F, *Body, F->getASTContext());
+    auto CFCtx = ControlFlowContext::build(*F, *Body, F->getASTContext());
     // FIXME: Handle errors.
     assert(CFCtx);
     auto Result = FunctionContexts.insert({F, std::move(*CFCtx)});
     return &Result.first->second;
   }
 
   return nullptr;
 }

clang/lib/Analysis/FlowSensitive/Transfer.cpp

public:
   }
 
   void VisitUnaryOperator(const UnaryOperator *S) {
     const Expr *SubExpr = S->getSubExpr();
     assert(SubExpr != nullptr);
 
     switch (S->getOpcode()) {
     case UO_Deref: {
-      // Skip past a reference to handle dereference of a dependent pointer.
-      const auto *SubExprVal = cast_or_null<PointerValue>(
-          Env.getValue(*SubExpr, SkipPast::Reference));
+      const auto *SubExprVal =
+          cast_or_null<PointerValue>(Env.getValue(*SubExpr, SkipPast::None));
       if (SubExprVal == nullptr)
         break;
 
       auto &Loc = Env.createStorageLocation(*S);
       Env.setStorageLocation(*S, Loc);
       Env.setValue(Loc,
                    Env.create<ReferenceValue>(SubExprVal->getPointeeLoc()));
       break;

clang/unittests/Analysis/FlowSensitive/TestingSupport.h

for (const ast_matchers::BoundNodes &BN :
     // Get the AST node of the target function.
     const FunctionDecl *Target = BN.getNodeAs<FunctionDecl>("target");
     if (Target == nullptr)
       return llvm::make_error<llvm::StringError>(
           llvm::errc::invalid_argument, "Could not find the target function.");
 
     // Build the control flow graph for the target function.
     auto MaybeCFCtx =
-        ControlFlowContext::build(Target, *Target->getBody(), Context);
+        ControlFlowContext::build(*Target, *Target->getBody(), Context);
     if (!MaybeCFCtx) return MaybeCFCtx.takeError();
     auto &CFCtx = *MaybeCFCtx;
 
     // Initialize states for running dataflow analysis.
     DataflowAnalysisContext DACtx(std::make_unique<WatchedLiteralsSolver>(),
                                   {/*Opts=*/AI.BuiltinOptions});
     Environment InitEnv(DACtx, *Target);
     auto Analysis = AI.MakeAnalysis(Context, InitEnv);

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

 using namespace test;
 using ::testing::IsNull;
 using ::testing::NotNull;
 using ::testing::UnorderedElementsAre;
 
 using BuiltinOptions = DataflowAnalysisContext::Options;
 
 template <typename Matcher>
-void runDataflow(llvm::StringRef Code, Matcher Match,
+llvm::Error
+runDataflowReturnError(llvm::StringRef Code, Matcher Match,
                        DataflowAnalysisOptions Options,
                        LangStandard::Kind Std = LangStandard::lang_cxx17,
                        llvm::StringRef TargetFun = "target") {
   using ast_matchers::hasName;
   llvm::SmallVector<std::string, 3> ASTBuildArgs = {
+      // -fnodelayed-template-parsing is the default everywhere but on Windows.
+      // Set it explicitly so that tests behave the same on Windows as on other
+      // platforms.
       "-fsyntax-only", "-fno-delayed-template-parsing",
       "-std=" +
           std::string(LangStandard::getLangStandardForKind(Std).getName())};
   AnalysisInputs<NoopAnalysis> AI(
       Code, hasName(TargetFun),
       [UseBuiltinModel = Options.BuiltinOpts.has_value()](ASTContext &C,
                                                           Environment &Env) {
         return NoopAnalysis(
             C,
             DataflowAnalysisOptions{
                 UseBuiltinModel ? Env.getDataflowAnalysisContext().getOptions()
                                 : std::optional<BuiltinOptions>()});
       });
   AI.ASTBuildArgs = ASTBuildArgs;
   if (Options.BuiltinOpts)
     AI.BuiltinOptions = *Options.BuiltinOpts;
-  ASSERT_THAT_ERROR(
-      checkDataflow<NoopAnalysis>(
-          std::move(AI),
-          /*VerifyResults=*/
-          [&Match](const llvm::StringMap<DataflowAnalysisState<NoopLattice>>
-                       &Results,
-                   const AnalysisOutputs &AO) { Match(Results, AO.ASTCtx); }),
+  return checkDataflow<NoopAnalysis>(
+      [&Match](
+          const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+          const AnalysisOutputs &AO) { Match(Results, AO.ASTCtx); });
+}
+
+template <typename Matcher>
+void runDataflow(llvm::StringRef Code, Matcher Match,
+                 DataflowAnalysisOptions Options,
+                 LangStandard::Kind Std = LangStandard::lang_cxx17,
+                 llvm::StringRef TargetFun = "target") {
+  ASSERT_THAT_ERROR(
+      runDataflowReturnError(Code, Match, Options, Std, TargetFun),
       llvm::Succeeded());
 }
 
 template <typename Matcher>
 void runDataflow(llvm::StringRef Code, Matcher Match,
                  LangStandard::Kind Std = LangStandard::lang_cxx17,
                  bool ApplyBuiltinTransfer = true,
                  llvm::StringRef TargetFun = "target") {
runDataflow(
         ASSERT_THAT(BarDecl, NotNull());
 
         const auto *FooVal = cast<PointerValue>(Env.getValue(*FooDecl));
         const auto *BarVal = cast<PointerValue>(Env.getValue(*BarDecl));
         EXPECT_EQ(&BarVal->getPointeeLoc(), &FooVal->getPointeeLoc());
       });
 }
 
-TEST(TransferTest, DerefDependentPtr) {
+TEST(TransferTest, CannotAnalyzeFunctionTemplate) {
   std::string Code = R"(
     template <typename T>
-    void target(T *Foo) {
-      T &Bar = *Foo;
-      /*[[p]]*/
-    }
+    void target() {}
   )";
-  runDataflow(
+  ASSERT_THAT_ERROR(
+      runDataflowReturnError(
           Code,
           [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
-         ASTContext &ASTCtx) {
-        ASSERT_THAT(Results.keys(), UnorderedElementsAre("p"));
-        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
-
-        const ValueDecl *FooDecl = findValueDecl(ASTCtx, "Foo");
-        ASSERT_THAT(FooDecl, NotNull());
-
-        const ValueDecl *BarDecl = findValueDecl(ASTCtx, "Bar");
-        ASSERT_THAT(BarDecl, NotNull());
+             ASTContext &ASTCtx) {},
+          {BuiltinOptions()}),
+      llvm::FailedWithMessage("Cannot analyze templated declarations"));
+}
 
-        const auto *FooVal = cast<PointerValue>(Env.getValue(*FooDecl));
-        const auto *BarLoc = Env.getStorageLocation(*BarDecl);
-        EXPECT_EQ(BarLoc, &FooVal->getPointeeLoc());
-      });
+TEST(TransferTest, CannotAnalyzeMethodOfClassTemplate) {
+  std::string Code = R"(
+    template <typename T>
+    struct A {
+      void target() {}
+    };
+  )";
+  ASSERT_THAT_ERROR(
+      runDataflowReturnError(
+          Code,
+          [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
+             ASTContext &ASTCtx) {},
+          {BuiltinOptions()}),
+      llvm::FailedWithMessage("Cannot analyze templated declarations"));
 }
 
 TEST(TransferTest, VarDeclInitAssignConditionalOperator) {
   std::string Code = R"(
     struct A {};
 
     void target(A Foo, A Bar, bool Cond) {
       A Baz = Cond ?  Foo : Bar;

clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

auto *Func = selectFirst<FunctionDecl>(
       "func", match(functionDecl(ast_matchers::hasName("target")).bind("func"),
                     AST->getASTContext()));
   assert(Func != nullptr);
 
   Stmt *Body = Func->getBody();
   assert(Body != nullptr);
 
   auto CFCtx = llvm::cantFail(
-      ControlFlowContext::build(nullptr, *Body, AST->getASTContext()));
+      ControlFlowContext::build(*Func, *Body, AST->getASTContext()));
 
   AnalysisT Analysis = MakeAnalysis(AST->getASTContext());
   DataflowAnalysisContext DACtx(std::make_unique<WatchedLiteralsSolver>());
   Environment Env(DACtx);
 
   return runDataflowAnalysis(CFCtx, Analysis, Env);
 }