[clang][tooling] Properly initialize DiagnosticsEngine for cc1 command-line construction

[jansvoboda11]

Summary

In ToolInvocation::run, the driver -> cc1 command-line transformation uses DiagnosticsEngine that wasn't completely initialized. This patch ensures ProcessWarningOptions(DiagnosticsEngine&, const DiagnosticOptions &) is called.

Depends on D108982.

[jansvoboda11]

XFAIL clang-scan-deps test

[dexonsmith]

clang/test/ClangScanDeps/strip_diag_serialize.cpp

1–3 ↗

(On Diff #369657)

Can you clarify why a temporary regression is necessary? Is there a way of reordering patches to avoid it?

[jansvoboda11]

Shuffling patches around.

[dexonsmith]

LGTM.

[dexonsmith]

(If you can squash the changes from https://reviews.llvm.org/D109158 into here, even better.)

clang/lib/Tooling/Tooling.cpp

bool ToolInvocation::run() {
   IntrusiveRefCntPtr<DiagnosticOptions> ParsedDiagOpts;
   DiagnosticOptions *DiagOpts = this->DiagOpts;
   if (!DiagOpts) {
     ParsedDiagOpts = CreateAndPopulateDiagOpts(Argv);
     DiagOpts = &*ParsedDiagOpts;
   }
 
   TextDiagnosticPrinter DiagnosticPrinter(llvm::errs(), DiagOpts);
-  DiagnosticsEngine Diagnostics(
-      IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs()), &*DiagOpts,
-      DiagConsumer ? DiagConsumer : &DiagnosticPrinter, false);
+  IntrusiveRefCntPtr<DiagnosticsEngine> Diagnostics =
+      CompilerInstance::createDiagnostics(
+          &*DiagOpts, DiagConsumer ? DiagConsumer : &DiagnosticPrinter, false);
   // Although `Diagnostics` are used only for command-line parsing, the custom
   // `DiagConsumer` might expect a `SourceManager` to be present.
-  SourceManager SrcMgr(Diagnostics, *Files);
-  Diagnostics.setSourceManager(&SrcMgr);
+  SourceManager SrcMgr(*Diagnostics, *Files);
+  Diagnostics->setSourceManager(&SrcMgr);
 
   const std::unique_ptr<driver::Driver> Driver(
-      newDriver(&Diagnostics, BinaryName, &Files->getVirtualFileSystem()));
+      newDriver(&*Diagnostics, BinaryName, &Files->getVirtualFileSystem()));
   // The "input file not found" diagnostics from the driver are useful.
   // The driver is only aware of the VFS working directory, but some clients
   // change this at the FileManager level instead.
   // In this case the checks have false positives, so skip them.
   if (!Files->getFileSystemOpts().WorkingDir.empty())
     Driver->setCheckInputsExist(false);
   const std::unique_ptr<driver::Compilation> Compilation(
       Driver->BuildCompilation(llvm::makeArrayRef(Argv)));
   if (!Compilation)
     return false;
   const llvm::opt::ArgStringList *const CC1Args = getCC1Arguments(
-      &Diagnostics, Compilation.get());
+      &*Diagnostics, Compilation.get());
   if (!CC1Args)
     return false;
   std::unique_ptr<CompilerInvocation> Invocation(
-      newInvocation(&Diagnostics, *CC1Args, BinaryName));
+      newInvocation(&*Diagnostics, *CC1Args, BinaryName));
   return runInvocation(BinaryName, Compilation.get(), std::move(Invocation),
                        std::move(PCHContainerOps));
 }
 
 bool ToolInvocation::runInvocation(
     const char *BinaryName, driver::Compilation *Compilation,
     std::shared_ptr<CompilerInvocation> Invocation,
     std::shared_ptr<PCHContainerOperations> PCHContainerOps) {

clang/unittests/Tooling/ToolingTest.cpp

InMemoryFileSystem->addFile("def/abc", 0,
                               llvm::MemoryBuffer::getMemBuffer("\n"));
   // Add a module.map file in the include directory of our header, so we trigger
   // the module.map header search logic.
   InMemoryFileSystem->addFile("def/module.map", 0,
                               llvm::MemoryBuffer::getMemBuffer("\n"));
   EXPECT_TRUE(Invocation.run());
 }
 
+struct ErrorCountingDiagnosticConsumer : public DiagnosticConsumer {
+  ErrorCountingDiagnosticConsumer() : NumErrorsSeen(0) {}
+  void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+                        const Diagnostic &Info) override {
+    if (DiagLevel == DiagnosticsEngine::Level::Error)
+      ++NumErrorsSeen;
+  }
+  unsigned NumErrorsSeen;
+};
+
+TEST(ToolInvocation, DiagnosticsEngineProperlyInitializedForCC1Construction) {
+  llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> OverlayFileSystem(
+      new llvm::vfs::OverlayFileSystem(llvm::vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFileSystem(
+      new llvm::vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+  llvm::IntrusiveRefCntPtr<FileManager> Files(
+      new FileManager(FileSystemOptions(), OverlayFileSystem));
+
+  std::vector<std::string> Args;
+  Args.push_back("tool-executable");
+  Args.push_back("-target");
+  // Invalid argument that by default results in an error diagnostic:
+  Args.push_back("i386-apple-ios14.0-simulator");
+  // Argument that downgrades the error into a warning:
+  Args.push_back("-Wno-error=invalid-ios-deployment-target");
+  Args.push_back("-fsyntax-only");
+  Args.push_back("test.cpp");
+
+  clang::tooling::ToolInvocation Invocation(
+      Args, std::make_unique<SyntaxOnlyAction>(), Files.get());
+  InMemoryFileSystem->addFile(
+      "test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int a() {}\n"));
+  ErrorCountingDiagnosticConsumer Consumer;
+  Invocation.setDiagnosticConsumer(&Consumer);
+  EXPECT_TRUE(Invocation.run());
+  // Check that `-Wno-error=invalid-ios-deployment-target` downgraded the error
+  // into a warning.
+  EXPECT_EQ(Consumer.NumErrorsSeen, 0u);
+}
+
+TEST(ToolInvocation, CustomDiagnosticOptionsOverwriteParsedOnes) {
+  llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> OverlayFileSystem(
+      new llvm::vfs::OverlayFileSystem(llvm::vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFileSystem(
+      new llvm::vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+  llvm::IntrusiveRefCntPtr<FileManager> Files(
+      new FileManager(FileSystemOptions(), OverlayFileSystem));
+
+  std::vector<std::string> Args;
+  Args.push_back("tool-executable");
+  Args.push_back("-target");
+  // Invalid argument that by default results in an error diagnostic:
+  Args.push_back("i386-apple-ios14.0-simulator");
+  // Argument that downgrades the error into a warning:
+  Args.push_back("-Wno-error=invalid-ios-deployment-target");
+  Args.push_back("-fsyntax-only");
+  Args.push_back("test.cpp");
+
+  clang::tooling::ToolInvocation Invocation(
+      Args, std::make_unique<SyntaxOnlyAction>(), Files.get());
+  InMemoryFileSystem->addFile(
+      "test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int a() {}\n"));
+  ErrorCountingDiagnosticConsumer Consumer;
+  Invocation.setDiagnosticConsumer(&Consumer);
+
+  auto DiagOpts = llvm::makeIntrusiveRefCnt<DiagnosticOptions>();
+  Invocation.setDiagnosticOptions(&*DiagOpts);
+
+  EXPECT_TRUE(Invocation.run());
+  // Check that `-Wno-error=invalid-ios-deployment-target` didn't downgrade the
+  // error into a warning due to being overwritten by custom diagnostic options.
+  EXPECT_EQ(Consumer.NumErrorsSeen, 1u);
+}
+
 struct DiagnosticConsumerExpectingSourceManager : public DiagnosticConsumer {
   bool SawSourceManager;
 
   DiagnosticConsumerExpectingSourceManager() : SawSourceManager(false) {}
 
   void HandleDiagnostic(clang::DiagnosticsEngine::Level,
                         const clang::Diagnostic &info) override {
     SawSourceManager = info.hasSourceManager();