-
Notifications
You must be signed in to change notification settings - Fork 14.9k
Frontend: Adopt llvm::vfs::OutputBackend in CompilerInstance #113364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Frontend: Adopt llvm::vfs::OutputBackend in CompilerInstance #113364
Conversation
Created using spr 1.3.5 [skip ci]
Created using spr 1.3.5
@llvm/pr-subscribers-clang Author: Steven Wu (cachemeifyoucan) ChangesAdopt new virtual output backend in CompilerInstance. Full diff: https://github.com/llvm/llvm-project/pull/113364.diff 2 Files Affected:
diff --git a/clang/include/clang/Frontend/CompilerInstance.h b/clang/include/clang/Frontend/CompilerInstance.h
index 3464654284f199..96bc6c9ed839b2 100644
--- a/clang/include/clang/Frontend/CompilerInstance.h
+++ b/clang/include/clang/Frontend/CompilerInstance.h
@@ -24,6 +24,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/BuryPointer.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/VirtualOutputBackend.h"
#include <cassert>
#include <list>
#include <memory>
@@ -92,6 +93,9 @@ class CompilerInstance : public ModuleLoader {
/// The file manager.
IntrusiveRefCntPtr<FileManager> FileMgr;
+ /// The output context.
+ IntrusiveRefCntPtr<llvm::vfs::OutputBackend> TheOutputBackend;
+
/// The source manager.
IntrusiveRefCntPtr<SourceManager> SourceMgr;
@@ -182,22 +186,8 @@ class CompilerInstance : public ModuleLoader {
/// The stream for verbose output.
raw_ostream *VerboseOutputStream = &llvm::errs();
- /// Holds information about the output file.
- ///
- /// If TempFilename is not empty we must rename it to Filename at the end.
- /// TempFilename may be empty and Filename non-empty if creating the temporary
- /// failed.
- struct OutputFile {
- std::string Filename;
- std::optional<llvm::sys::fs::TempFile> File;
-
- OutputFile(std::string filename,
- std::optional<llvm::sys::fs::TempFile> file)
- : Filename(std::move(filename)), File(std::move(file)) {}
- };
-
/// The list of active output files.
- std::list<OutputFile> OutputFiles;
+ std::list<llvm::vfs::OutputFile> OutputFiles;
/// Force an output buffer.
std::unique_ptr<llvm::raw_pwrite_stream> OutputStream;
@@ -459,6 +449,22 @@ class CompilerInstance : public ModuleLoader {
/// Replace the current file manager and virtual file system.
void setFileManager(FileManager *Value);
+ /// @name Output Backend.
+ /// {
+
+ /// Set the output backend.
+ void
+ setOutputBackend(IntrusiveRefCntPtr<llvm::vfs::OutputBackend> NewOutputs);
+
+ /// Create an output manager.
+ void createOutputBackend();
+
+ bool hasOutputBackend() const { return bool(TheOutputBackend); }
+
+ llvm::vfs::OutputBackend &getOutputBackend();
+ llvm::vfs::OutputBackend &getOrCreateOutputBackend();
+
+ /// }
/// @}
/// @name Source Manager
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 240305b33824b8..a1d7d330039504 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -54,6 +54,8 @@
#include "llvm/Support/Signals.h"
#include "llvm/Support/TimeProfiler.h"
#include "llvm/Support/Timer.h"
+#include "llvm/Support/VirtualOutputBackends.h"
+#include "llvm/Support/VirtualOutputError.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/TargetParser/Host.h"
#include <optional>
@@ -514,6 +516,10 @@ void CompilerInstance::createPreprocessor(TranslationUnitKind TUKind) {
collectVFSEntries(*this, ModuleDepCollector);
}
+ // Modules need an output manager.
+ if (!hasOutputBackend())
+ createOutputBackend();
+
for (auto &Listener : DependencyCollectors)
Listener->attachToPreprocessor(*PP);
@@ -769,32 +775,19 @@ void CompilerInstance::createSema(TranslationUnitKind TUKind,
void CompilerInstance::clearOutputFiles(bool EraseFiles) {
// The ASTConsumer can own streams that write to the output files.
assert(!hasASTConsumer() && "ASTConsumer should be reset");
- // Ignore errors that occur when trying to discard the temp file.
- for (OutputFile &OF : OutputFiles) {
- if (EraseFiles) {
- if (OF.File)
- consumeError(OF.File->discard());
- if (!OF.Filename.empty())
- llvm::sys::fs::remove(OF.Filename);
- continue;
- }
-
- if (!OF.File)
- continue;
-
- if (OF.File->TmpName.empty()) {
- consumeError(OF.File->discard());
- continue;
- }
-
- llvm::Error E = OF.File->keep(OF.Filename);
- if (!E)
- continue;
-
- getDiagnostics().Report(diag::err_unable_to_rename_temp)
- << OF.File->TmpName << OF.Filename << std::move(E);
-
- llvm::sys::fs::remove(OF.File->TmpName);
+ if (!EraseFiles) {
+ for (auto &O : OutputFiles)
+ llvm::handleAllErrors(
+ O.keep(),
+ [&](const llvm::vfs::TempFileOutputError &E) {
+ getDiagnostics().Report(diag::err_unable_to_rename_temp)
+ << E.getTempPath() << E.getOutputPath()
+ << E.convertToErrorCode().message();
+ },
+ [&](const llvm::vfs::OutputError &E) {
+ getDiagnostics().Report(diag::err_fe_unable_to_open_output)
+ << E.getOutputPath() << E.convertToErrorCode().message();
+ });
}
OutputFiles.clear();
if (DeleteBuiltModules) {
@@ -828,6 +821,29 @@ std::unique_ptr<raw_pwrite_stream> CompilerInstance::createNullOutputFile() {
return std::make_unique<llvm::raw_null_ostream>();
}
+void CompilerInstance::setOutputBackend(
+ IntrusiveRefCntPtr<llvm::vfs::OutputBackend> NewOutputs) {
+ assert(!TheOutputBackend && "Already has an output manager");
+ TheOutputBackend = std::move(NewOutputs);
+}
+
+void CompilerInstance::createOutputBackend() {
+ assert(!TheOutputBackend && "Already has an output manager");
+ TheOutputBackend =
+ llvm::makeIntrusiveRefCnt<llvm::vfs::OnDiskOutputBackend>();
+}
+
+llvm::vfs::OutputBackend &CompilerInstance::getOutputBackend() {
+ assert(TheOutputBackend);
+ return *TheOutputBackend;
+}
+
+llvm::vfs::OutputBackend &CompilerInstance::getOrCreateOutputBackend() {
+ if (!hasOutputBackend())
+ createOutputBackend();
+ return getOutputBackend();
+}
+
std::unique_ptr<raw_pwrite_stream>
CompilerInstance::createOutputFile(StringRef OutputPath, bool Binary,
bool RemoveFileOnSignal, bool UseTemporary,
@@ -862,98 +878,20 @@ CompilerInstance::createOutputFileImpl(StringRef OutputPath, bool Binary,
OutputPath = *AbsPath;
}
- std::unique_ptr<llvm::raw_fd_ostream> OS;
- std::optional<StringRef> OSFile;
-
- if (UseTemporary) {
- if (OutputPath == "-")
- UseTemporary = false;
- else {
- llvm::sys::fs::file_status Status;
- llvm::sys::fs::status(OutputPath, Status);
- if (llvm::sys::fs::exists(Status)) {
- // Fail early if we can't write to the final destination.
- if (!llvm::sys::fs::can_write(OutputPath))
- return llvm::errorCodeToError(
- make_error_code(llvm::errc::operation_not_permitted));
-
- // Don't use a temporary if the output is a special file. This handles
- // things like '-o /dev/null'
- if (!llvm::sys::fs::is_regular_file(Status))
- UseTemporary = false;
- }
- }
- }
-
- std::optional<llvm::sys::fs::TempFile> Temp;
- if (UseTemporary) {
- // Create a temporary file.
- // Insert -%%%%%%%% before the extension (if any), and because some tools
- // (noticeable, clang's own GlobalModuleIndex.cpp) glob for build
- // artifacts, also append .tmp.
- StringRef OutputExtension = llvm::sys::path::extension(OutputPath);
- SmallString<128> TempPath =
- StringRef(OutputPath).drop_back(OutputExtension.size());
- TempPath += "-%%%%%%%%";
- TempPath += OutputExtension;
- TempPath += ".tmp";
- llvm::sys::fs::OpenFlags BinaryFlags =
- Binary ? llvm::sys::fs::OF_None : llvm::sys::fs::OF_Text;
- Expected<llvm::sys::fs::TempFile> ExpectedFile =
- llvm::sys::fs::TempFile::create(
- TempPath, llvm::sys::fs::all_read | llvm::sys::fs::all_write,
- BinaryFlags);
-
- llvm::Error E = handleErrors(
- ExpectedFile.takeError(), [&](const llvm::ECError &E) -> llvm::Error {
- std::error_code EC = E.convertToErrorCode();
- if (CreateMissingDirectories &&
- EC == llvm::errc::no_such_file_or_directory) {
- StringRef Parent = llvm::sys::path::parent_path(OutputPath);
- EC = llvm::sys::fs::create_directories(Parent);
- if (!EC) {
- ExpectedFile = llvm::sys::fs::TempFile::create(
- TempPath, llvm::sys::fs::all_read | llvm::sys::fs::all_write,
- BinaryFlags);
- if (!ExpectedFile)
- return llvm::errorCodeToError(
- llvm::errc::no_such_file_or_directory);
- }
- }
- return llvm::errorCodeToError(EC);
- });
-
- if (E) {
- consumeError(std::move(E));
- } else {
- Temp = std::move(ExpectedFile.get());
- OS.reset(new llvm::raw_fd_ostream(Temp->FD, /*shouldClose=*/false));
- OSFile = Temp->TmpName;
- }
- // If we failed to create the temporary, fallback to writing to the file
- // directly. This handles the corner case where we cannot write to the
- // directory, but can write to the file.
- }
-
- if (!OS) {
- OSFile = OutputPath;
- std::error_code EC;
- OS.reset(new llvm::raw_fd_ostream(
- *OSFile, EC,
- (Binary ? llvm::sys::fs::OF_None : llvm::sys::fs::OF_TextWithCRLF)));
- if (EC)
- return llvm::errorCodeToError(EC);
- }
-
- // Add the output file -- but don't try to remove "-", since this means we are
- // using stdin.
- OutputFiles.emplace_back(((OutputPath != "-") ? OutputPath : "").str(),
- std::move(Temp));
-
- if (!Binary || OS->supportsSeeking())
- return std::move(OS);
-
- return std::make_unique<llvm::buffer_unique_ostream>(std::move(OS));
+ using namespace llvm::vfs;
+ Expected<OutputFile> O = getOrCreateOutputBackend().createFile(
+ OutputPath,
+ OutputConfig()
+ .setTextWithCRLF(!Binary)
+ .setDiscardOnSignal(RemoveFileOnSignal)
+ .setAtomicWrite(UseTemporary)
+ .setImplyCreateDirectories(UseTemporary && CreateMissingDirectories));
+ if (!O)
+ return O.takeError();
+
+ O->discardOnDestroy([](llvm::Error E) { consumeError(std::move(E)); });
+ OutputFiles.push_back(std::move(*O));
+ return OutputFiles.back().createProxy();
}
// Initialization Utilities
|
This seems good, perhaps after #113363 lands, it'd be interesting to rebase and run some build-time tests, just to ensure it doesn't introduce regressions. |
Yup! We don't see anything on our end but it will be good to double check. |
Before landing the other change and rebase this, I created a branch to benchmark compile time: https://llvm-compile-time-tracker.com/compare.php?from=3b3fc701d8f83d4ca30ee1c818fb7687336ac178&to=e5febfb1d9d880b4f7af5dc32194f479ee2adcd6&stat=instructions%3Au Nothing stands out as regression. |
Created using spr 1.3.6 [skip ci]
Created using spr 1.3.6
✅ With the latest revision this PR passed the C/C++ code formatter. |
Created using spr 1.3.6
Created using spr 1.3.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Assuming there’s no negative compile time impact, this looks like a nice clean up.
Adopt new virtual output backend in CompilerInstance.