Author: Jan Svoboda Date: 2025-12-16T08:54:42-08:00 New Revision: 0badd4e2cb217a36e718a7e62c537810489bfc92
URL: https://github.com/llvm/llvm-project/commit/0badd4e2cb217a36e718a7e62c537810489bfc92 DIFF: https://github.com/llvm/llvm-project/commit/0badd4e2cb217a36e718a7e62c537810489bfc92.diff LOG: [clang][deps] Prefer `DiagnosticConsumer` over `llvm::{Error,Expected}` (#172389) This PR moves the `DependencyScanning{Worker,Tool}` APIs from using `llvm::{Error,Expected}` to using `DiagnosticConsumer`. This makes it possible to learn about non-error diagnostics (warnings, remarks) emitted by the scanner. I found a need for this while exploring the possibility of caching dependency scans and emitting remarks to report cache hits/misses. Added: Modified: clang-tools-extra/clangd/ScanningProjectModules.cpp clang/include/clang/DependencyScanning/DependencyScanningWorker.h clang/include/clang/Tooling/DependencyScanningTool.h clang/lib/DependencyScanning/DependencyScanningWorker.cpp clang/lib/Tooling/DependencyScanningTool.cpp clang/test/ClangScanDeps/error.cpp clang/tools/clang-scan-deps/ClangScanDeps.cpp clang/unittests/Tooling/DependencyScannerTest.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/ScanningProjectModules.cpp b/clang-tools-extra/clangd/ScanningProjectModules.cpp index e1b6cbe1ae818..860ce377be532 100644 --- a/clang-tools-extra/clangd/ScanningProjectModules.cpp +++ b/clang-tools-extra/clangd/ScanningProjectModules.cpp @@ -110,12 +110,18 @@ ModuleDependencyScanner::scan(PathRef FilePath, llvm::sys::path::remove_filename(FilePathDir); DependencyScanningTool ScanningTool(Service, TFS.view(FilePathDir)); - llvm::Expected<P1689Rule> ScanningResult = - ScanningTool.getP1689ModuleDependencyFile(Cmd, Cmd.Directory); - - if (auto E = ScanningResult.takeError()) { - elog("Scanning modules dependencies for {0} failed: {1}", FilePath, - llvm::toString(std::move(E))); + std::string S; + llvm::raw_string_ostream OS(S); + DiagnosticOptions DiagOpts; + DiagOpts.ShowCarets = false; + TextDiagnosticPrinter DiagConsumer(OS, DiagOpts); + + std::optional<P1689Rule> ScanningResult = + ScanningTool.getP1689ModuleDependencyFile(Cmd, Cmd.Directory, + DiagConsumer); + + if (!ScanningResult) { + elog("Scanning modules dependencies for {0} failed: {1}", FilePath, S); return std::nullopt; } diff --git a/clang/include/clang/DependencyScanning/DependencyScanningWorker.h b/clang/include/clang/DependencyScanning/DependencyScanningWorker.h index 489fba4ed3f6b..3c5e7660a8c7b 100644 --- a/clang/include/clang/DependencyScanning/DependencyScanningWorker.h +++ b/clang/include/clang/DependencyScanning/DependencyScanningWorker.h @@ -107,16 +107,6 @@ class DependencyScanningWorker { DiagnosticConsumer &DiagConsumer, std::optional<llvm::MemoryBufferRef> TUBuffer = std::nullopt); - /// Run the dependency scanning tool for a given clang driver command-line - /// for a specific translation unit via file system or memory buffer. - /// - /// \returns A \c StringError with the diagnostic output if clang errors - /// occurred, success otherwise. - llvm::Error computeDependencies( - StringRef WorkingDirectory, ArrayRef<std::string> CommandLine, - DependencyConsumer &Consumer, DependencyActionController &Controller, - std::optional<llvm::MemoryBufferRef> TUBuffer = std::nullopt); - /// The three method below implements a new interface for by name /// dependency scanning. They together enable the dependency scanning worker /// to more effectively perform scanning for a sequence of modules diff --git a/clang/include/clang/Tooling/DependencyScanningTool.h b/clang/include/clang/Tooling/DependencyScanningTool.h index 415dcf17c37b9..82e73bdc9580a 100644 --- a/clang/include/clang/Tooling/DependencyScanningTool.h +++ b/clang/include/clang/Tooling/DependencyScanningTool.h @@ -45,10 +45,11 @@ class DependencyScanningTool { /// file format that is specified in the options (-MD is the default) and /// return it. /// - /// \returns A \c StringError with the diagnostic output if clang errors - /// occurred, dependency file contents otherwise. - llvm::Expected<std::string> - getDependencyFile(ArrayRef<std::string> CommandLine, StringRef CWD); + /// \returns std::nullopt if errors occurred (reported to the DiagConsumer), + /// dependency file contents otherwise. + std::optional<std::string> + getDependencyFile(ArrayRef<std::string> CommandLine, StringRef CWD, + DiagnosticConsumer &DiagConsumer); /// Collect the module dependency in P1689 format for C++20 named modules. /// @@ -59,19 +60,21 @@ class DependencyScanningTool { /// \param MakeformatOutputPath The output parameter for the path to /// \param MakeformatOutput. /// - /// \returns A \c StringError with the diagnostic output if clang errors - /// occurred, P1689 dependency format rules otherwise. - llvm::Expected<P1689Rule> + /// \returns std::nullopt if errors occurred (reported to the DiagConsumer), + /// P1689 dependency format rules otherwise. + std::optional<P1689Rule> getP1689ModuleDependencyFile(const CompileCommand &Command, StringRef CWD, std::string &MakeformatOutput, - std::string &MakeformatOutputPath); - llvm::Expected<P1689Rule> - getP1689ModuleDependencyFile(const CompileCommand &Command, StringRef CWD) { + std::string &MakeformatOutputPath, + DiagnosticConsumer &DiagConsumer); + std::optional<P1689Rule> + getP1689ModuleDependencyFile(const CompileCommand &Command, StringRef CWD, + DiagnosticConsumer &DiagConsumer) { std::string MakeformatOutput; std::string MakeformatOutputPath; return getP1689ModuleDependencyFile(Command, CWD, MakeformatOutput, - MakeformatOutputPath); + MakeformatOutputPath, DiagConsumer); } /// Given a Clang driver command-line for a translation unit, gather the @@ -89,11 +92,12 @@ class DependencyScanningTool { /// TUBuffer is nullopt, the input should be included in the /// Commandline already. /// - /// \returns a \c StringError with the diagnostic output if clang errors - /// occurred, \c TranslationUnitDeps otherwise. - llvm::Expected<dependencies::TranslationUnitDeps> + /// \returns std::nullopt if errors occurred (reported to the DiagConsumer), + /// translation unit dependencies otherwise. + std::optional<dependencies::TranslationUnitDeps> getTranslationUnitDependencies( ArrayRef<std::string> CommandLine, StringRef CWD, + DiagnosticConsumer &DiagConsumer, const llvm::DenseSet<dependencies::ModuleID> &AlreadySeen, dependencies::LookupModuleOutputCallback LookupModuleOutput, std::optional<llvm::MemoryBufferRef> TUBuffer = std::nullopt); diff --git a/clang/lib/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/DependencyScanning/DependencyScanningWorker.cpp index 99fe082634524..ea2a6d67948c3 100644 --- a/clang/lib/DependencyScanning/DependencyScanningWorker.cpp +++ b/clang/lib/DependencyScanning/DependencyScanningWorker.cpp @@ -40,21 +40,6 @@ DependencyScanningWorker::DependencyScanningWorker( DependencyScanningWorker::~DependencyScanningWorker() = default; DependencyActionController::~DependencyActionController() = default; -llvm::Error DependencyScanningWorker::computeDependencies( - StringRef WorkingDirectory, ArrayRef<std::string> CommandLine, - DependencyConsumer &Consumer, DependencyActionController &Controller, - std::optional<llvm::MemoryBufferRef> TUBuffer) { - // Capture the emitted diagnostics and report them to the client - // in the case of a failure. - TextDiagnosticsPrinterWithOutput DiagPrinterWithOS(CommandLine); - - if (computeDependencies(WorkingDirectory, CommandLine, Consumer, Controller, - DiagPrinterWithOS.DiagPrinter, TUBuffer)) - return llvm::Error::success(); - return llvm::make_error<llvm::StringError>( - DiagPrinterWithOS.DiagnosticsOS.str(), llvm::inconvertibleErrorCode()); -} - static bool forEachDriverJob( ArrayRef<std::string> ArgStrs, DiagnosticsEngine &Diags, IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS, diff --git a/clang/lib/Tooling/DependencyScanningTool.cpp b/clang/lib/Tooling/DependencyScanningTool.cpp index a69d996551a5c..986b4166bebd4 100644 --- a/clang/lib/Tooling/DependencyScanningTool.cpp +++ b/clang/lib/Tooling/DependencyScanningTool.cpp @@ -74,23 +74,23 @@ class MakeDependencyPrinterConsumer : public DependencyConsumer { }; } // anonymous namespace -llvm::Expected<std::string> +std::optional<std::string> DependencyScanningTool::getDependencyFile(ArrayRef<std::string> CommandLine, - StringRef CWD) { - MakeDependencyPrinterConsumer Consumer; + StringRef CWD, + DiagnosticConsumer &DiagConsumer) { + MakeDependencyPrinterConsumer DepConsumer; CallbackActionController Controller(nullptr); - auto Result = - Worker.computeDependencies(CWD, CommandLine, Consumer, Controller); - if (Result) - return std::move(Result); + if (!Worker.computeDependencies(CWD, CommandLine, DepConsumer, Controller, + DiagConsumer)) + return std::nullopt; std::string Output; - Consumer.printDependencies(Output); + DepConsumer.printDependencies(Output); return Output; } -llvm::Expected<P1689Rule> DependencyScanningTool::getP1689ModuleDependencyFile( +std::optional<P1689Rule> DependencyScanningTool::getP1689ModuleDependencyFile( const CompileCommand &Command, StringRef CWD, std::string &MakeformatOutput, - std::string &MakeformatOutputPath) { + std::string &MakeformatOutputPath, DiagnosticConsumer &DiagConsumer) { class P1689ModuleDependencyPrinterConsumer : public MakeDependencyPrinterConsumer { public: @@ -132,10 +132,9 @@ llvm::Expected<P1689Rule> DependencyScanningTool::getP1689ModuleDependencyFile( P1689Rule Rule; P1689ModuleDependencyPrinterConsumer Consumer(Rule, Command); P1689ActionController Controller; - auto Result = Worker.computeDependencies(CWD, Command.CommandLine, Consumer, - Controller); - if (Result) - return std::move(Result); + if (!Worker.computeDependencies(CWD, Command.CommandLine, Consumer, + Controller, DiagConsumer)) + return std::nullopt; MakeformatOutputPath = Consumer.getMakeFormatDependencyOutputPath(); if (!MakeformatOutputPath.empty()) @@ -143,19 +142,19 @@ llvm::Expected<P1689Rule> DependencyScanningTool::getP1689ModuleDependencyFile( return Rule; } -llvm::Expected<TranslationUnitDeps> +std::optional<TranslationUnitDeps> DependencyScanningTool::getTranslationUnitDependencies( ArrayRef<std::string> CommandLine, StringRef CWD, + DiagnosticConsumer &DiagConsumer, const llvm::DenseSet<ModuleID> &AlreadySeen, LookupModuleOutputCallback LookupModuleOutput, std::optional<llvm::MemoryBufferRef> TUBuffer) { FullDependencyConsumer Consumer(AlreadySeen); CallbackActionController Controller(LookupModuleOutput); - llvm::Error Result = Worker.computeDependencies(CWD, CommandLine, Consumer, - Controller, TUBuffer); - if (Result) - return std::move(Result); + if (!Worker.computeDependencies(CWD, CommandLine, Consumer, Controller, + DiagConsumer, TUBuffer)) + return std::nullopt; return Consumer.takeTranslationUnitDeps(); } diff --git a/clang/test/ClangScanDeps/error.cpp b/clang/test/ClangScanDeps/error.cpp index 593dbf35edca5..03126d8812613 100644 --- a/clang/test/ClangScanDeps/error.cpp +++ b/clang/test/ClangScanDeps/error.cpp @@ -7,7 +7,7 @@ // RUN: not clang-scan-deps -- %clang -c %t/missing_tu.c 2>%t/missing_tu.errs // RUN: echo EOF >> %t/missing_tu.errs // RUN: cat %t/missing_tu.errs | sed 's:\\\\\?:/:g' | FileCheck %s --check-prefix=CHECK-MISSING-TU -DPREFIX=%/t -// CHECK-MISSING-TU: Error while scanning dependencies for [[PREFIX]]/missing_tu.c +// CHECK-MISSING-TU: Diagnostics while scanning dependencies for '[[PREFIX]]/missing_tu.c': // CHECK-MISSING-TU-NEXT: error: no such file or directory: '[[PREFIX]]/missing_tu.c' // CHECK-MISSING-TU-NEXT: error: no input files // CHECK-MISSING-TU-NEXT: error: @@ -16,6 +16,6 @@ // RUN: not clang-scan-deps -- %clang -c %t/missing_header.c 2>%t/missing_header.errs // RUN: echo EOF >> %t/missing_header.errs // RUN: cat %t/missing_header.errs | sed 's:\\\\\?:/:g' | FileCheck %s --check-prefix=CHECK-MISSING-HEADER -DPREFIX=%/t -// CHECK-MISSING-HEADER: Error while scanning dependencies for [[PREFIX]]/missing_header.c -// CHECK-MISSING-HEADER-NEXT: fatal error: 'missing.h' file not found +// CHECK-MISSING-HEADER: Diagnostics while scanning dependencies for '[[PREFIX]]/missing_header.c': +// CHECK-MISSING-HEADER-NEXT: [[PREFIX]]/missing_header.c:1:10: fatal error: 'missing.h' file not found // CHECK-MISSING-HEADER-NEXT: EOF diff --git a/clang/tools/clang-scan-deps/ClangScanDeps.cpp b/clang/tools/clang-scan-deps/ClangScanDeps.cpp index 9acd0aca737ba..256a70568a3bf 100644 --- a/clang/tools/clang-scan-deps/ClangScanDeps.cpp +++ b/clang/tools/clang-scan-deps/ClangScanDeps.cpp @@ -325,26 +325,16 @@ class ResourceDirectoryCache { } // end anonymous namespace -/// Takes the result of a dependency scan and prints error / dependency files -/// based on the result. -/// -/// \returns True on error. -static bool -handleMakeDependencyToolResult(const std::string &Input, - llvm::Expected<std::string> &MaybeFile, - SharedStream &OS, SharedStream &Errs) { - if (!MaybeFile) { - llvm::handleAllErrors( - MaybeFile.takeError(), [&Input, &Errs](llvm::StringError &Err) { - Errs.applyLocked([&](raw_ostream &OS) { - OS << "Error while scanning dependencies for " << Input << ":\n"; - OS << Err.getMessage(); - }); - }); - return true; - } - OS.applyLocked([&](raw_ostream &OS) { OS << *MaybeFile; }); - return false; +/// Prints any diagnostics produced during a dependency scan. +static void handleDiagnostics(StringRef Input, StringRef Diagnostics, + SharedStream &Errs) { + if (Diagnostics.empty()) + return; + + Errs.applyLocked([&](raw_ostream &OS) { + OS << "Diagnostics while scanning dependencies for '" << Input << "':\n"; + OS << Diagnostics; + }); } template <typename Container> @@ -627,23 +617,6 @@ class FullDeps { std::vector<InputDeps> Inputs; }; -static bool handleTranslationUnitResult( - StringRef Input, llvm::Expected<TranslationUnitDeps> &MaybeTUDeps, - FullDeps &FD, size_t InputIndex, SharedStream &OS, SharedStream &Errs) { - if (!MaybeTUDeps) { - llvm::handleAllErrors( - MaybeTUDeps.takeError(), [&Input, &Errs](llvm::StringError &Err) { - Errs.applyLocked([&](raw_ostream &OS) { - OS << "Error while scanning dependencies for " << Input << ":\n"; - OS << Err.getMessage(); - }); - }); - return true; - } - FD.mergeDeps(Input, std::move(*MaybeTUDeps), InputIndex); - return false; -} - static bool handleModuleResult(StringRef ModuleName, llvm::Expected<TranslationUnitDeps> &MaybeTUDeps, FullDeps &FD, size_t InputIndex, @@ -741,24 +714,6 @@ class P1689Deps { std::vector<P1689Rule> Rules; }; -static bool -handleP1689DependencyToolResult(const std::string &Input, - llvm::Expected<P1689Rule> &MaybeRule, - P1689Deps &PD, SharedStream &Errs) { - if (!MaybeRule) { - llvm::handleAllErrors( - MaybeRule.takeError(), [&Input, &Errs](llvm::StringError &Err) { - Errs.applyLocked([&](raw_ostream &OS) { - OS << "Error while scanning dependencies for " << Input << ":\n"; - OS << Err.getMessage(); - }); - }); - return true; - } - PD.addRules(*MaybeRule); - return false; -} - /// Construct a path for the explicitly built PCM. static std::string constructPCMPath(ModuleID MID, StringRef OutputDir) { SmallString<256> ExplicitPCMPath(OutputDir); @@ -1036,6 +991,12 @@ int clang_scan_deps_main(int argc, char **argv, const llvm::ToolContext &) { std::string Filename = std::move(Input->Filename); std::string CWD = std::move(Input->Directory); + std::string S; + llvm::raw_string_ostream OS(S); + DiagnosticOptions DiagOpts; + DiagOpts.ShowCarets = false; + TextDiagnosticPrinter DiagConsumer(OS, DiagOpts); + std::string OutputDir(ModuleFilesDir); if (OutputDir.empty()) OutputDir = getModuleCachePath(Input->CommandLine); @@ -1045,9 +1006,12 @@ int clang_scan_deps_main(int argc, char **argv, const llvm::ToolContext &) { // Run the tool on it. if (Format == ScanningOutputFormat::Make) { - auto MaybeFile = WorkerTool.getDependencyFile(Input->CommandLine, CWD); - if (handleMakeDependencyToolResult(Filename, MaybeFile, DependencyOS, - Errs)) + auto MaybeFile = + WorkerTool.getDependencyFile(Input->CommandLine, CWD, DiagConsumer); + handleDiagnostics(Filename, S, Errs); + if (MaybeFile) + DependencyOS.applyLocked([&](raw_ostream &OS) { OS << *MaybeFile; }); + else HadErrors = true; } else if (Format == ScanningOutputFormat::P1689) { // It is useful to generate the make-format dependency output during @@ -1058,9 +1022,11 @@ int clang_scan_deps_main(int argc, char **argv, const llvm::ToolContext &) { std::string MakeformatOutput; auto MaybeRule = WorkerTool.getP1689ModuleDependencyFile( - *Input, CWD, MakeformatOutput, MakeformatOutputPath); - - if (handleP1689DependencyToolResult(Filename, MaybeRule, PD, Errs)) + *Input, CWD, MakeformatOutput, MakeformatOutputPath, DiagConsumer); + handleDiagnostics(Filename, S, Errs); + if (MaybeRule) + PD.addRules(*MaybeRule); + else HadErrors = true; if (!MakeformatOutputPath.empty() && !MakeformatOutput.empty() && @@ -1086,10 +1052,8 @@ int clang_scan_deps_main(int argc, char **argv, const llvm::ToolContext &) { } SharedStream MakeformatOS(OSIter->second); - llvm::Expected<std::string> MaybeOutput(MakeformatOutput); - if (handleMakeDependencyToolResult(Filename, MaybeOutput, - MakeformatOS, Errs)) - HadErrors = true; + MakeformatOS.applyLocked( + [&](raw_ostream &OS) { OS << MakeformatOutput; }); } } else if (ModuleNames) { StringRef ModuleNameRef(*ModuleNames); @@ -1150,10 +1114,12 @@ int clang_scan_deps_main(int argc, char **argv, const llvm::ToolContext &) { Filename = TU->getBufferIdentifier(); } auto MaybeTUDeps = WorkerTool.getTranslationUnitDependencies( - Input->CommandLine, CWD, AlreadySeenModules, LookupOutput, - TUBuffer); - if (handleTranslationUnitResult(Filename, MaybeTUDeps, *FD, LocalIndex, - DependencyOS, Errs)) + Input->CommandLine, CWD, DiagConsumer, AlreadySeenModules, + LookupOutput, TUBuffer); + handleDiagnostics(Filename, S, Errs); + if (MaybeTUDeps) + FD->mergeDeps(Filename, *MaybeTUDeps, LocalIndex); + else HadErrors = true; } } diff --git a/clang/unittests/Tooling/DependencyScannerTest.cpp b/clang/unittests/Tooling/DependencyScannerTest.cpp index da47252b37097..3874b40ff8461 100644 --- a/clang/unittests/Tooling/DependencyScannerTest.cpp +++ b/clang/unittests/Tooling/DependencyScannerTest.cpp @@ -14,6 +14,7 @@ #include "clang/Frontend/CompilerInstance.h" #include "clang/Frontend/FrontendAction.h" #include "clang/Frontend/FrontendActions.h" +#include "clang/Frontend/TextDiagnosticBuffer.h" #include "clang/Tooling/CompilationDatabase.h" #include "clang/Tooling/DependencyScanningTool.h" #include "clang/Tooling/Tooling.h" @@ -235,12 +236,11 @@ TEST(DependencyScanner, ScanDepsWithFS) { ScanningOutputFormat::Make); DependencyScanningTool ScanTool(Service, VFS); - std::string DepFile; - ASSERT_THAT_ERROR( - ScanTool.getDependencyFile(CommandLine, CWD).moveInto(DepFile), - llvm::Succeeded()); - using llvm::sys::path::convert_to_slash; - EXPECT_EQ(convert_to_slash(DepFile), + TextDiagnosticBuffer DiagConsumer; + std::optional<std::string> DepFile = + ScanTool.getDependencyFile(CommandLine, CWD, DiagConsumer); + ASSERT_TRUE(DepFile.has_value()); + EXPECT_EQ(llvm::sys::path::convert_to_slash(*DepFile), "test.cpp.o: /root/test.cpp /root/header.h\n"); } @@ -296,10 +296,10 @@ TEST(DependencyScanner, ScanDepsWithModuleLookup) { // This will fail with "fatal error: module 'Foo' not found" but it doesn't // matter, the point of the test is to check that files are not read // unnecessarily. - std::string DepFile; - ASSERT_THAT_ERROR( - ScanTool.getDependencyFile(CommandLine, CWD).moveInto(DepFile), - llvm::Failed()); + TextDiagnosticBuffer DiagConsumer; + std::optional<std::string> DepFile = + ScanTool.getDependencyFile(CommandLine, CWD, DiagConsumer); + ASSERT_FALSE(DepFile.has_value()); EXPECT_TRUE(!llvm::is_contained(InterceptFS->StatPaths, OtherPath)); EXPECT_EQ(InterceptFS->ReadFiles, std::vector<std::string>{"test.m"}); _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
