Support: Change FileOutputBuffer to return FileError

[dexonsmith]

Summary

Update FileOutputBuffer to always return a FileError, now that it will
pass through the error code from the wrapped error. Also update the
callers not to redundantly include the filename in messages.

One awkward question is whether sys::fs::TempFile should return
FileError. If TempFile starts doing so and it reports the temporary file
path, then FileOutputBuffer should stop adding one... but it probably
wants to avoid leaking the temporary name and report the intended output
file instead. Maybe createFileError() should avoid double-wrapping a
FileError, instead replacing the named file in that error?

Another awkward question is whether all the callers were updated to
avoid redundantly reporting the filename. No tests failed, even before I
audited the callers. Maybe this is fine?

Note: this is an example of using https://reviews.llvm.org/D113225.

[dblaikie]

Maybe createFileError() should avoid double-wrapping a FileError, instead replacing the named file in that error?

Got examples of what the double-wrapping looks like? Maybe compared to how it looks with other tools that might encounter similar issues?

I'd guess changing file names could result in incorrect messages, though - suggesting that, say, a directory is read-only but the final target directory isn't read only and it's really an issue with the temp file location - so maybe it'd be best to keep the double wrapping?

Another awkward question is whether all the callers were updated to avoid redundantly reporting the filename. No tests failed, even before I audited the callers. Maybe this is fine?

Better test coverage is always nice - but I'll admit I can be a bit lax on error paths. Maybe changing the error paths to "assert(false)" could help identify tests for them (hopefully there are some) that could be fleshed out to check the actual error message more narrowly.

[int3]

I assume this is being abandoned

lld/COFF/Writer.cpp

template <typename PEHeaderTy> void Writer::writeHeader() {
   // The first 4 bytes is length including itself.
   buf = reinterpret_cast<uint8_t *>(&symbolTable[numberOfSymbols]);
   write32le(buf, strtab.size() + 4);
   if (!strtab.empty())
     memcpy(buf + 4, strtab.data(), strtab.size());
 }
 
 void Writer::openFile(StringRef path) {
-  buffer = CHECK(
-      FileOutputBuffer::create(path, fileSize, FileOutputBuffer::F_executable),
-      "failed to open " + path);
+  if (Error E = FileOutputBuffer::create(path, fileSize,
+                                         FileOutputBuffer::F_executable)
+                    .moveInto(buffer))
+    fatal("failed to open " + llvm::toString(std::move(E)));
 }
 
 void Writer::createSEHTable() {
   SymbolRVASet handlers;
   for (ObjFile *file : ctx.objFileInstances) {
     if (!file->hasSafeSEH())
       error("/safeseh: " + file->getName() + " is not compatible with SEH");
     markSymbolsForRVATable(file, file->getSXDataChunks(), handlers);

lld/ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::openFile() {
   if (!config->relocatable)
     flags |= FileOutputBuffer::F_executable;
   if (!config->mmapOutputFile)
     flags |= FileOutputBuffer::F_no_mmap;
   Expected<std::unique_ptr<FileOutputBuffer>> bufferOrErr =
       FileOutputBuffer::create(config->outputFile, fileSize, flags);
 
   if (!bufferOrErr) {
-    error("failed to open " + config->outputFile + ": " +
-          llvm::toString(bufferOrErr.takeError()));
+    error("failed to open " + llvm::toString(bufferOrErr.takeError()));
     return;
   }
   buffer = std::move(*bufferOrErr);
   Out::bufferStart = buffer->getBufferStart();
 }
 
 template <class ELFT> void Writer<ELFT>::writeSectionsBinary() {
   for (OutputSection *sec : outputSections)

lld/MachO/Writer.cpp

 }
 
 void Writer::openFile() {
   Expected<std::unique_ptr<FileOutputBuffer>> bufferOrErr =
       FileOutputBuffer::create(config->outputFile, fileOff,
                                FileOutputBuffer::F_executable);
 
   if (!bufferOrErr)
-    error("failed to open " + config->outputFile + ": " +
-          llvm::toString(bufferOrErr.takeError()));
+    error("failed to open " + llvm::toString(bufferOrErr.takeError()));
   else
     buffer = std::move(*bufferOrErr);
 }
 
 void Writer::writeSections() {
   uint8_t *buf = buffer->getBufferStart();
   for (const OutputSegment *seg : outputSegments)
     for (const OutputSection *osec : seg->getSections())

lld/wasm/Writer.cpp

 void Writer::openFile() {
   log("writing: " + config->outputFile);
 
   Expected<std::unique_ptr<FileOutputBuffer>> bufferOrErr =
       FileOutputBuffer::create(config->outputFile, fileSize,
                                FileOutputBuffer::F_executable);
 
   if (!bufferOrErr)
-    error("failed to open " + config->outputFile + ": " +
-          toString(bufferOrErr.takeError()));
+    error("failed to open " + toString(bufferOrErr.takeError()));
   else
     buffer = std::move(*bufferOrErr);
 }
 
 void Writer::createHeader() {
   raw_string_ostream os(header);
   writeBytes(os, WasmMagic, sizeof(WasmMagic), "wasm magic");
   writeU32(os, WasmVersion, "wasm version");
   os.flush();
   fileSize += header.size();
 }
 
 void writeResult() { Writer().run(); }
 
 } // namespace wasm
 } // namespace lld

llvm/include/llvm/Support/FileOutputBuffer.h

public:
   /// buffer of the specified size. When committed, the buffer will be written
   /// to the file at the specified path.
   ///
   /// When F_modify is specified and \p FilePath refers to an existing on-disk
   /// file \p Size may be set to -1, in which case the entire file is used.
   /// Otherwise, the file shrinks or grows as necessary based on the value of
   /// \p Size.  It is an error to specify F_modify and Size=-1 if \p FilePath
   /// does not exist.
+  ///
+  /// Returns a \a FileError pointing at \p FilePath on failure.
   static Expected<std::unique_ptr<FileOutputBuffer>>
   create(StringRef FilePath, size_t Size, unsigned Flags = 0);
 
   /// Returns a pointer to the start of the buffer.
   virtual uint8_t *getBufferStart() const = 0;
 
   /// Returns a pointer to the end of the buffer.
   virtual uint8_t *getBufferEnd() const = 0;
 
   /// Returns size of the buffer.
   virtual size_t getBufferSize() const = 0;
 
   /// Returns path where file will show up if buffer is committed.
   StringRef getPath() const { return FinalPath; }
 
   /// Flushes the content of the buffer to its file and deallocates the
   /// buffer.  If commit() is not called before this object's destructor
   /// is called, the file is deleted in the destructor. The optional parameter
   /// is used if it turns out you want the file size to be smaller than
   /// initially requested.
+  ///
+  /// Returns a \a FileError pointing at \a getPath() on failure.
   virtual Error commit() = 0;
 
   /// If this object was previously committed, the destructor just deletes
   /// this object.  If this object was not committed, the destructor
   /// deallocates the buffer and the target file is never written.
   virtual ~FileOutputBuffer() {}
 
   /// This removes the temporary file (unless it already was committed)

llvm/lib/InterfaceStub/ELFObjHandler.cpp

if (ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrError =
         return Error::success();
     }
   }
 
   Expected<std::unique_ptr<FileOutputBuffer>> BufOrError =
       FileOutputBuffer::create(FilePath, Builder.getSize());
   if (!BufOrError)
     return createStringError(errc::invalid_argument,
-                             toString(BufOrError.takeError()) +
-                                 " when trying to open `" + FilePath +
-                                 "` for writing");
+                             "failed to open " +
+                                 toString(BufOrError.takeError()));
 
   // Write binary to file.
   std::unique_ptr<FileOutputBuffer> FileBuf = std::move(*BufOrError);
   memcpy(FileBuf->getBufferStart(), Buf.data(), Buf.size());
 
   return FileBuf->commit();
 }
 

llvm/lib/Support/FileOutputBuffer.cpp

public:
 
   size_t getBufferSize() const override { return Buffer.size(); }
 
   Error commit() override {
     // Unmap buffer, letting OS flush dirty pages to file on disk.
     Buffer.unmap();
 
     // Atomically replace the existing file with the new one.
-    return Temp.keep(FinalPath);
+    return createFileError(FinalPath, Temp.keep(FinalPath));
   }
 
   ~OnDiskBuffer() override {
     // Close the mapping before deleting the temp file, so that the removal
     // succeeds.
     Buffer.unmap();
     consumeError(Temp.discard());
   }
if (FinalPath == "-") {
       return Error::success();
     }
 
     using namespace sys::fs;
     int FD;
     std::error_code EC;
     if (auto EC =
             openFileForWrite(FinalPath, FD, CD_CreateAlways, OF_None, Mode))
-      return errorCodeToError(EC);
+      return createFileError(FinalPath, EC);
     raw_fd_ostream OS(FD, /*shouldClose=*/true, /*unbuffered=*/true);
     OS << StringRef((const char *)Buffer.base(), BufferSize);
     return Error::success();
   }
 
 private:
   // Buffer may actually contain a larger memory block than BufferSize
   OwningMemoryBlock Buffer;
   size_t BufferSize;
   unsigned Mode;
 };
 } // namespace
 
 static Expected<std::unique_ptr<InMemoryBuffer>>
 createInMemoryBuffer(StringRef Path, size_t Size, unsigned Mode) {
   std::error_code EC;
   MemoryBlock MB = Memory::allocateMappedMemory(
       Size, nullptr, sys::Memory::MF_READ | sys::Memory::MF_WRITE, EC);
   if (EC)
-    return errorCodeToError(EC);
+    return createFileError(Path, EC);
   return std::make_unique<InMemoryBuffer>(Path, MB, Size, Mode);
 }
 
 static Expected<std::unique_ptr<FileOutputBuffer>>
 createOnDiskBuffer(StringRef Path, size_t Size, unsigned Mode) {
   Expected<fs::TempFile> FileOrErr =
       fs::TempFile::create(Path + ".tmp%%%%%%%", Mode);
   if (!FileOrErr)
-    return FileOrErr.takeError();
+    return createFileError(Path, FileOrErr.takeError());
   fs::TempFile File = std::move(*FileOrErr);
 
   if (auto EC = fs::resize_file_before_mapping_readwrite(File.FD, Size)) {
     consumeError(File.discard());
-    return errorCodeToError(EC);
+    return createFileError(Path, EC);
   }
 
   // Mmap it.
   std::error_code EC;
   fs::mapped_file_region MappedFile =
       fs::mapped_file_region(fs::convertFDToNativeFile(File.FD),
                              fs::mapped_file_region::readwrite, Size, 0, EC);
 
FileOutputBuffer::create(StringRef Path, size_t Size, unsigned Flags) {
   // by rename(2).
   //
   // However, if the destination file is a special file, we don't want to
   // use rename (e.g. we don't want to replace /dev/null with a regular
   // file.) If that's the case, we create an in-memory buffer, open the
   // destination file and write to it on commit().
   switch (Stat.type()) {
   case fs::file_type::directory_file:
-    return errorCodeToError(errc::is_a_directory);
+    return createFileError(Path, errc::is_a_directory);
   case fs::file_type::regular_file:
   case fs::file_type::file_not_found:
   case fs::file_type::status_error:
     if (Flags & F_no_mmap)
       return createInMemoryBuffer(Path, Size, Mode);
     else
       return createOnDiskBuffer(Path, Size, Mode);
   default:
     return createInMemoryBuffer(Path, Size, Mode);
   }
 }

llvm/tools/llvm-cvtres/llvm-cvtres.cpp

 };
 }
 
 [[noreturn]] static void reportError(Twine Msg) {
   errs() << Msg;
   exit(1);
 }
 
-static void reportError(StringRef Input, std::error_code EC) {
-  reportError(Twine(Input) + ": " + EC.message() + ".\n");
-}
-
 static void error(StringRef Input, Error EC) {
   if (!EC)
     return;
   handleAllErrors(std::move(EC), [&](const ErrorInfoBase &EI) {
     reportError(Twine(Input) + ": " + EI.message() + ".\n");
   });
 }
 
int main(int Argc, const char **Argv) {
 
   if (Verbose) {
     Parser.printTree(outs());
   }
 
   std::unique_ptr<MemoryBuffer> OutputBuffer =
       error(llvm::object::writeWindowsResourceCOFF(MachineType, Parser,
                                                    DateTimeStamp));
-  auto FileOrErr =
-      FileOutputBuffer::create(OutputFile, OutputBuffer->getBufferSize());
-  if (!FileOrErr)
-    reportError(OutputFile, errorToErrorCode(FileOrErr.takeError()));
-  std::unique_ptr<FileOutputBuffer> FileBuffer = std::move(*FileOrErr);
+  std::unique_ptr<FileOutputBuffer> FileBuffer = error(
+      FileOutputBuffer::create(OutputFile, OutputBuffer->getBufferSize()));
   std::copy(OutputBuffer->getBufferStart(), OutputBuffer->getBufferEnd(),
             FileBuffer->getBufferStart());
   error(FileBuffer->commit());
 
   if (Verbose) {
     std::unique_ptr<MemoryBuffer> Buffer =
         error(OutputFile,
               MemoryBuffer::getFileOrSTDIN(OutputFile, /*IsText=*/false,

llvm/tools/llvm-lipo/llvm-lipo.cpp

[[noreturn]] static void thinSlice(ArrayRef<OwningBinary<Binary>> InputBinaries,
 
   Expected<std::unique_ptr<FileOutputBuffer>> OutFileOrError =
       FileOutputBuffer::create(OutputFileName,
                                B->getMemoryBufferRef().getBufferSize(),
                                sys::fs::can_execute(UO->getFileName())
                                    ? FileOutputBuffer::F_executable
                                    : 0);
   if (!OutFileOrError)
-    reportError(OutputFileName, OutFileOrError.takeError());
+    reportError(OutFileOrError.takeError());
   std::copy(B->getMemoryBufferRef().getBufferStart(),
             B->getMemoryBufferRef().getBufferEnd(),
             OutFileOrError.get()->getBufferStart());
   if (Error E = OutFileOrError.get()->commit())
     reportError(OutputFileName, std::move(E));
   exit(EXIT_SUCCESS);
 }
 

llvm/tools/llvm-mt/llvm-mt.cpp

for (const auto &File : InputFiles) {
     error(Merger.merge(*ManifestOrErr.get()));
   }
 
   std::unique_ptr<MemoryBuffer> OutputBuffer = Merger.getMergedManifest();
   if (!OutputBuffer)
     reportError("empty manifest not written");
   Expected<std::unique_ptr<FileOutputBuffer>> FileOrErr =
       FileOutputBuffer::create(OutputFile, OutputBuffer->getBufferSize());
-  if (!FileOrErr)
-    reportError(OutputFile, errorToErrorCode(FileOrErr.takeError()));
+  error(FileOrErr.takeError());
   std::unique_ptr<FileOutputBuffer> FileBuffer = std::move(*FileOrErr);
   std::copy(OutputBuffer->getBufferStart(), OutputBuffer->getBufferEnd(),
             FileBuffer->getBufferStart());
   error(FileBuffer->commit());
   return 0;
 }

llvm/unittests/Support/FileOutputBufferTest.cpp

 //===- llvm/unittest/Support/FileOutputBuffer.cpp - unit tests ------------===//
 //
 // 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 "llvm/Support/FileOutputBuffer.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/Testing/Support/Error.h"
+#include "llvm/Testing/Support/SupportHelpers.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 using namespace llvm::sys;
 
 #define ASSERT_NO_ERROR(x)                                                     \
   if (std::error_code ASSERT_NO_ERROR_ec = x) {                                \
     SmallString<128> MessageStorage;                                           \