[clang] Generalize WrapperFrontendAction to support multiple actions

[jkorous]

Summary

Split off functionality used in index-while-building feature as suggested in the review
https://reviews.llvm.org/D64384

[gribozavr]

clang/lib/Frontend/FrontendAction.cpp

1130

I think it is very difficult to combine these capability flags like hasIRSupport and hasCodeCompletionSupport. It seems to me that all wrapped actions need to precisely agree on values of these flags, otherwise at least one of the actions will be run in a mode that it does not expect.

For example, take hasCodeCompletionSupport. Let's say we have two actions, one turns it on, another turns it off. If the combined wrapper decides to turn code completion support on, then the non-CC action will be unhappy because parsing will be cut off in the middle. If the combined wrapper decides to turn CC off, the CC action will be unhappy because parsing will not be interrupted.

That was a hypothetical example, but a real example that does come up in the index-while-building patch is getTranslationUnitKind. GenerateIndexAction wants it to be TU_Complete, but IndexingFrontendAction can set it to TU_Prefix. When they disagree, there's no way to reconcile in a generic way, unless we try to understand what these specific actions do.

Therefore, it seems like combining FrontendActions is not a great idea in general... I'm sorry for suggesting it, I didn't realize it at that point.

Where we combine FrontendActions right now, things mostly happen to work by chance, and when wrappers forward these capability bits to the wrapped action, they often assume certain values for these bits anyway.

I wanted to suggest something different -- using ASTConsumer as the base currency, and trying to combine abstractions. Let's look at GenerateIndexAction and IndexingFrontendAction in more detail.

GenerateIndexAction is an ASTConsumer plus GenerateIndexAction::EndSourceFileAction callback. EndSourceFileAction, I think, is roughly equivalent to ASTConsumer::HandleTranslationUnit. Therefore, GenerateIndexAction does not need to be a FrontendAction, it is just an ASTConsumer.

IndexingFrontendAction is a lot more complex -- not only it needs a complex factory that creates the ASTConsumer, but it also overrides getTranslationUnitKind, which changes the behavior of Sema and Preprocessor in significant ways. So it does not seem like IndexFrontendAction can become an ASTConsumer.

However, IndexFrontendAction does not seem like a standalone ASTConsumer anyway. Its primary reason to exist is the logic in IndexingConsumer::shouldSkipFunctionBody. That logic seems interesting for general-purpose indexing, not just for libclang, so it would make sense to fold it into IndexASTConsumer.

To summarize:

• Fold shouldSkipBody from IndexingConsumer in libclang into IndexASTConsumer in libIndex.

• Make libIndex expose an ASTConsumer factory function, that encapsulates the logic currently in IndexingFrontendAction::CreateASTConsumer. Clients who need to combine indexing with some other FrontendAction can use that factory to create a consumer that will index whatever comes out of Sema.

• Make a FrontendAction that implements the getTranslationUnitKind function to implement the corresponding optimization. If this FrontendAction is only needed in libclang, it can live there, otherwise, it can be in libIndex.

What do you think?

[jkorous]

clang/lib/Frontend/FrontendAction.cpp

1130

I agree that this abstraction would be more on the pragmatic end of spectrum rather than on the end where suitable and fitting abstractions live. Both implementations and clients would have to be really careful to not violate some implicit assumptions. I'll give it another go.

I'm definitely in favor of simplifying the design if possible. Thanks for the suggestions! I'll try to refactor the code to get some feel for what can be done here.

[jkorous]

I'll just leave this review here until we finish the discussion but consider it to be effectively abandoned.

clang/lib/Frontend/FrontendAction.cpp

1130

Thanks for the suggestion, I didn't realize the correspondence between ASTConsumer::HandleTranslationUnit and FrontendAction::EndSourceFileAction.

I changed Emit|Generate-IndexAction to ASTConsumer in the original patch.
https://reviews.llvm.org/D64384

Now, I'm not sure I understand your intentions about moving shouldSkipFunctionBody from IndexingConsumer in libclang to (IIUC) GenerateIndexASTConsumer in libIndex as GenerateIndexASTConsumer. (Using terms of https://reviews.llvm.org/D64384).

Or do you mean just exposing the TUSkipBodyControl machinery in libIndex?

[gribozavr]

clang/lib/Frontend/FrontendAction.cpp

1130

Thank you!

1130

Sorry for being unclear -- yes, I meant exposing the APIs to skip function bodies in libIndex. Whether that involves exposing TUSkipBodyControl -- I don't know, maybe a simpler abstraction would be more general -- like a callback that, based on a source location, decides whether code at that declaration should be indexed.

clangd would be also interested, this patch just went in: https://reviews.llvm.org/D66226

[jkorous]

BTW It just occurred to me that multiplexing ASTConsumer::shouldSkipFunctionBody must have hit the same issue as FrontendAction multiplexing we considered above.

bool MultiplexConsumer::shouldSkipFunctionBody(Decl *D) {
  bool Skip = true;
  for (auto &Consumer : Consumers)
    Skip = Skip && Consumer->shouldSkipFunctionBody(D);
  return Skip;
}

[ilya-biryukov]

In D66125#1642013, @jkorous wrote:

BTW It just occurred to me that multiplexing ASTConsumer::shouldSkipFunctionBody must have hit the same issue as FrontendAction multiplexing we considered above.

bool MultiplexConsumer::shouldSkipFunctionBody(Decl *D) {
  bool Skip = true;
  for (auto &Consumer : Consumers)
    Skip = Skip && Consumer->shouldSkipFunctionBody(D);
  return Skip;
}

It's true that this is somewhat sketchy in the case when one of the consumers absolutely needs to skip the function body.
But the current implementation seems like the right trade-off in most (all?) of the practical use-cases: we should always prefer to skip function bodies because that makes clang faster; however, if any of the consumers do need the function body (e.g. the collect references, perform analyses on the function body, etc.), we cannot apply this optimization because that would introduce correctness bugs.

(sorry about chiming in the middle of the review, I've thought about this myself before and wanted to share my conclusions)

[gribozavr]

Agreed about multiplexing issues -- this is a case where wrapping or merging abstraction layers is the right solution.

[jkorous]

@ilya-biryukov @gribozavr I am with you, just mentioning this.

[jkorous]

Will take a look at getTranslationUnitKind optimization now.

clang/lib/Frontend/FrontendAction.cpp

1130

I put up this patch:
[clang][index] Move ParsedSrcLocationsTracker to libIndex
https://reviews.llvm.org/D66764

clang/include/clang/Frontend/FrontendAction.h

protected:
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  StringRef InFile) override;
 
 public:
   bool usesPreprocessorOnly() const override { return true; }
 };
 
 /// A frontend action which simply wraps some other runtime-specified
-/// frontend action.
+/// frontend actions.
+///
+/// Owns a list of FrontendAction-s and implements FrontendAction interface by
+/// forwarding to actions in the list (in the order they were provided) -
+/// with the exceptions of:
+/// - ExecuteAction(): First action's method is called only.
+/// - getTranslationUnitKind: Result of the first action's method is returned.
+///
+/// For methods returning bool we return boolean product of either particular
+/// values or their negation depending on the semantics.
+/// We don't exit early if some of the methods returns false as those
+/// could have side effects.
 ///
 /// Deriving from this class allows an action to inject custom logic around
-/// some existing action's behavior. It implements every virtual method in
-/// the FrontendAction interface by forwarding to the wrapped action.
+/// some existing action's behavior.
 class WrapperFrontendAction : public FrontendAction {
-  std::unique_ptr<FrontendAction> WrappedAction;
+  std::vector<std::unique_ptr<FrontendAction>> Actions;
 
 protected:
   bool PrepareToExecuteAction(CompilerInstance &CI) override;
+  /// \r consumer that forwards to consumers produced by actions in the list.
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  StringRef InFile) override;
   bool BeginInvocation(CompilerInstance &CI) override;
   bool BeginSourceFileAction(CompilerInstance &CI) override;
+  /// Invokes ExecuteAction of the first action in the list.
   void ExecuteAction() override;
   void EndSourceFileAction() override;
 
 public:
-  /// Construct a WrapperFrontendAction from an existing action, taking
-  /// ownership of it.
-  WrapperFrontendAction(std::unique_ptr<FrontendAction> WrappedAction);
+  WrapperFrontendAction(std::initializer_list<FrontendAction *> ActionList);
+  WrapperFrontendAction(std::unique_ptr<FrontendAction> Action);
 
+  /// Does ANY of the actions use preprocessor only?
   bool usesPreprocessorOnly() const override;
+  /// \r getTranslationUnitKind() of the first action or TU_Complete if there
+  /// are no actions.
   TranslationUnitKind getTranslationUnitKind() override;
+  /// Do ALL the actions support use with PCH?
   bool hasPCHSupport() const override;
+  /// Do ALL the actions support use with AST files?
   bool hasASTFileSupport() const override;
+  /// Do ALL the actions support use with IR file?
   bool hasIRSupport() const override;
+  /// Do ALL the actions support use with code completion?
   bool hasCodeCompletionSupport() const override;
 };
 
 }  // end namespace clang
 
 #endif

clang/lib/Frontend/FrontendAction.cpp

 
 std::unique_ptr<ASTConsumer>
 PreprocessorFrontendAction::CreateASTConsumer(CompilerInstance &CI,
                                               StringRef InFile) {
   llvm_unreachable("Invalid CreateASTConsumer on preprocessor action!");
 }
 
 bool WrapperFrontendAction::PrepareToExecuteAction(CompilerInstance &CI) {
-  return WrappedAction->PrepareToExecuteAction(CI);
+  bool Result = true;
+  for (auto &Action : Actions)
+    Result &= Action->PrepareToExecuteAction(CI);
+  return Result;
 }
 std::unique_ptr<ASTConsumer>
 WrapperFrontendAction::CreateASTConsumer(CompilerInstance &CI,
                                          StringRef InFile) {
-  return WrappedAction->CreateASTConsumer(CI, InFile);
+  std::vector<std::unique_ptr<ASTConsumer>> Consumers;
+  for (auto &Action : Actions) {
+    if (!Action->usesPreprocessorOnly())
+      Consumers.push_back(Action->CreateASTConsumer(CI, InFile));
+  }
+
+  return llvm::make_unique<MultiplexConsumer>(std::move(Consumers));
 }
 bool WrapperFrontendAction::BeginInvocation(CompilerInstance &CI) {
-  return WrappedAction->BeginInvocation(CI);
+  bool Result = true;
+  for (auto &Action : Actions)
+    Result &= Action->BeginInvocation(CI);
+  return Result;
 }
 bool WrapperFrontendAction::BeginSourceFileAction(CompilerInstance &CI) {
-  WrappedAction->setCurrentInput(getCurrentInput());
-  WrappedAction->setCompilerInstance(&CI);
-  auto Ret = WrappedAction->BeginSourceFileAction(CI);
+  auto CurrentInput = getCurrentInput();
+  bool Result = true;
+  for (auto &Action : Actions) {
+    Action->setCurrentInput(getCurrentInput());
+    Action->setCompilerInstance(&CI);
+    Result &= Action->BeginSourceFileAction(CI);
+  }
   // BeginSourceFileAction may change CurrentInput, e.g. during module builds.
-  setCurrentInput(WrappedAction->getCurrentInput());
-  return Ret;
+  setCurrentInput(CurrentInput);
+  return Result;
 }
 void WrapperFrontendAction::ExecuteAction() {
-  WrappedAction->ExecuteAction();
+  if (Actions.size() > 0)
+    return Actions.front()->ExecuteAction();
 }
 void WrapperFrontendAction::EndSourceFileAction() {
-  WrappedAction->EndSourceFileAction();
+  for (auto &Action : Actions)
+    Action->EndSourceFileAction();
 }
 
 bool WrapperFrontendAction::usesPreprocessorOnly() const {
-  return WrappedAction->usesPreprocessorOnly();
+  // The default is that we don't assume anything special.
+  bool Result = false;
+  for (auto &Action : Actions)
+    Result |= Action->usesPreprocessorOnly();
+  return Result;
 }
 TranslationUnitKind WrapperFrontendAction::getTranslationUnitKind() {
-  return WrappedAction->getTranslationUnitKind();
+  if (Actions.size() > 0)
+    return Actions[0]->getTranslationUnitKind();
+  return TU_Complete;
 }
 bool WrapperFrontendAction::hasPCHSupport() const {
-  return WrappedAction->hasPCHSupport();
+  bool Result = true;
+  for (auto &Action : Actions)
+    Result &= Action->hasPCHSupport();
+  return Result;
 }
 bool WrapperFrontendAction::hasASTFileSupport() const {
-  return WrappedAction->hasASTFileSupport();
+  bool Result = true;
+  for (auto &Action : Actions)
+    Result &= Action->hasASTFileSupport();
+  return Result;
 }
 bool WrapperFrontendAction::hasIRSupport() const {
-  return WrappedAction->hasIRSupport();
+  bool Result = true;
+  for (auto &Action : Actions)
+    Result &= Action->hasIRSupport();
+  return Result;
 }
 bool WrapperFrontendAction::hasCodeCompletionSupport() const {
-  return WrappedAction->hasCodeCompletionSupport();
+  bool Result = true;
+  for (auto &Action : Actions)
+    Result &= Action->hasCodeCompletionSupport();
+  return Result;
 }
 
 WrapperFrontendAction::WrapperFrontendAction(
-    std::unique_ptr<FrontendAction> WrappedAction)
-  : WrappedAction(std::move(WrappedAction)) {}
+    std::initializer_list<FrontendAction *> ActionList) {
+  Actions.reserve(ActionList.size());
+  for (FrontendAction *Action : ActionList)
+    Actions.emplace_back(Action);
+}
 
+WrapperFrontendAction::WrapperFrontendAction(
+    std::unique_ptr<FrontendAction> Action) {
+  Actions.emplace_back(std::move(Action));
+}

clang/unittests/Frontend/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_unittest(FrontendTests
   ASTUnitTest.cpp
   CompilerInstanceTest.cpp
   FixedPointString.cpp
   FrontendActionTest.cpp
   CodeGenActionTest.cpp
   ParsedSourceLocationTest.cpp
   PCHPreambleTest.cpp
   OutputStreamTest.cpp
+  WrapperFrontendActionTest.cpp
   )
 clang_target_link_libraries(FrontendTests
   PRIVATE
   clangAST
   clangBasic
   clangFrontend
   clangLex
   clangSema
   clangCodeGen
   clangFrontendTool
   clangSerialization
   )

clang/unittests/Frontend/WrapperFrontendActionTest.cpp

+//===- unittests/Frontend/WrapperFrontendActionTest.cpp -------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "clang/Sema/Sema.h"
+#include "clang/Serialization/InMemoryModuleCache.h"
+#include "llvm/ADT/Triple.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/ToolOutputFile.h"
+#include "gtest/gtest.h"
+#include <string>
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+class LoggingASTConsumer : public ASTConsumer {
+public:
+  LoggingASTConsumer(const std::string &ID, std::string &Log)
+      : ID(ID), Log(Log) {}
+
+  const std::string &ID;
+  std::string &Log;
+
+  void HandleTranslationUnit(ASTContext &context) override {
+    Log += "LoggingASTConsumer-" + ID + "-HandleTranslationUnit";
+  }
+};
+
+class LoggingASTFrontendAction : public ASTFrontendAction {
+public:
+  LoggingASTFrontendAction(const std::string &ID, std::string &Log)
+      : ID(ID), Log(Log) {}
+
+  const std::string &ID;
+  std::string &Log;
+
+  bool BeginSourceFileAction(CompilerInstance &CI) override {
+    setCompilerInstance(&CI);
+    CI.resetAndLeakPreprocessor();
+    Log += "LoggingASTFrontendAction-" + ID + "-BeginSourceFileAction";
+    return ASTFrontendAction::BeginSourceFileAction(CI);
+  }
+
+  std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
+                                                 StringRef InFile) override {
+    return llvm::make_unique<LoggingASTConsumer>(ID, Log);
+  }
+};
+
+class TestASTFrontendAction : public ASTFrontendAction {
+public:
+  TestASTFrontendAction(bool enableIncrementalProcessing = false,
+                        bool actOnEndOfTranslationUnit = false)
+      : EnableIncrementalProcessing(enableIncrementalProcessing),
+        ActOnEndOfTranslationUnit(actOnEndOfTranslationUnit) {}
+
+  bool EnableIncrementalProcessing;
+  bool ActOnEndOfTranslationUnit;
+  std::vector<std::string> decl_names;
+
+  bool BeginSourceFileAction(CompilerInstance &ci) override {
+    if (EnableIncrementalProcessing)
+      ci.getPreprocessor().enableIncrementalProcessing();
+
+    return ASTFrontendAction::BeginSourceFileAction(ci);
+  }
+
+  std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
+                                                 StringRef InFile) override {
+    return llvm::make_unique<Visitor>(CI, ActOnEndOfTranslationUnit,
+                                      decl_names);
+  }
+
+private:
+  class Visitor : public ASTConsumer, public RecursiveASTVisitor<Visitor> {
+  public:
+    Visitor(CompilerInstance &CI, bool ActOnEndOfTranslationUnit,
+            std::vector<std::string> &decl_names)
+        : CI(CI), ActOnEndOfTranslationUnit(ActOnEndOfTranslationUnit),
+          decl_names_(decl_names) {}
+
+    void HandleTranslationUnit(ASTContext &context) override {
+      if (ActOnEndOfTranslationUnit) {
+        CI.getSema().ActOnEndOfTranslationUnit();
+      }
+      TraverseDecl(context.getTranslationUnitDecl());
+    }
+
+    virtual bool VisitNamedDecl(NamedDecl *Decl) {
+      decl_names_.push_back(Decl->getQualifiedNameAsString());
+      return true;
+    }
+
+  private:
+    CompilerInstance &CI;
+    bool ActOnEndOfTranslationUnit;
+    std::vector<std::string> &decl_names_;
+  };
+};
+
+TEST(WrapperFrontendActionTest, Trivial) {
+  auto invocation = std::make_shared<CompilerInvocation>();
+  invocation->getPreprocessorOpts().addRemappedFile(
+      "test.cc",
+      MemoryBuffer::getMemBuffer("int main() { float x; }").release());
+  invocation->getFrontendOpts().Inputs.push_back(
+      FrontendInputFile("test.cc", InputKind::CXX));
+  invocation->getFrontendOpts().ProgramAction = frontend::ParseSyntaxOnly;
+  invocation->getTargetOpts().Triple = "i386-unknown-linux-gnu";
+  CompilerInstance compiler;
+  compiler.setInvocation(std::move(invocation));
+  compiler.createDiagnostics();
+
+  std::string ActionLog;
+  LoggingASTFrontendAction Action("A", ActionLog);
+
+  ASSERT_TRUE(compiler.ExecuteAction(Action));
+}
+
+TEST(WrapperFrontendActionTest, Order) {
+  auto invocation = std::make_shared<CompilerInvocation>();
+  invocation->getPreprocessorOpts().addRemappedFile(
+      "test.cc",
+      MemoryBuffer::getMemBuffer("int main() { float x; }").release());
+  invocation->getFrontendOpts().Inputs.push_back(
+      FrontendInputFile("test.cc", InputKind::CXX));
+  invocation->getFrontendOpts().ProgramAction = frontend::ParseSyntaxOnly;
+  invocation->getTargetOpts().Triple = "i386-unknown-linux-gnu";
+  CompilerInstance compiler;
+  compiler.setInvocation(std::move(invocation));
+  compiler.createDiagnostics();
+
+  std::string ActionLog;
+  WrapperFrontendAction Action({new LoggingASTFrontendAction("A", ActionLog),
+                                new LoggingASTFrontendAction("B", ActionLog),
+                                new LoggingASTFrontendAction("C", ActionLog)});
+
+  ASSERT_TRUE(compiler.ExecuteAction(Action));
+  EXPECT_EQ(ActionLog, "LoggingASTFrontendAction-A-BeginSourceFileAction"
+                       "LoggingASTFrontendAction-B-BeginSourceFileAction"
+                       "LoggingASTFrontendAction-C-BeginSourceFileAction"
+                       "LoggingASTConsumer-A-HandleTranslationUnit"
+                       "LoggingASTConsumer-B-HandleTranslationUnit"
+                       "LoggingASTConsumer-C-HandleTranslationUnit");
+}
+
+} // anonymous namespace