[raw_fd_ostream] report actual error in error messages

[inglorion]

Summary

Previously, we would emit error messages like "IO failure on output
stream". This change causes use to include information about what
actually went wrong, e.g. "No space left on device".

[inglorion]

As an alternative, I considered using an Error instead of an error_code. I decided against that, because

• It produces a riskier change. Error.h depends on raw_ostream.h, and now we would also have an edge going the other way. Working around this is possible, but can affect clients that depend on Error.h, of which there are many.
• It changes the semantics from unchecked errors resulting in fatals to unchecked errors *or successes* resulting in fatals. This could be worked around, but it would make the change larger comparing to just using a type that already provides compatible semantics.
• raw_ostream et al. are meant to look like standard C++ types. std::error_code is arguably a better fit for that than llvm::Error.
• The code that raw_fd_ostream calls already produces error_codes, so by just using those, we avoid having to convert them to Error.

In short, std::error_code makes for a simpler, less risky change that fits with the existing code and semantics, so I chose to go with that.

[rnk]

In D39203#904129, @inglorion wrote:

As an alternative, I considered using an Error instead of an error_code. I decided against that, because

• It produces a riskier change. Error.h depends on raw_ostream.h, and now we would also have an edge going the other way. Working around this is possible, but can affect clients that depend on Error.h, of which there are many.

Separately, we should cut this edge if possible. Widely included headers like Error.h should have as few transitive includes as possible.

• It changes the semantics from unchecked errors resulting in fatals to unchecked errors *or successes* resulting in fatals. This could be worked around, but it would make the change larger comparing to just using a type that already provides compatible semantics.
• raw_ostream et al. are meant to look like standard C++ types. std::error_code is arguably a better fit for that than llvm::Error.
• The code that raw_fd_ostream calls already produces error_codes, so by just using those, we avoid having to convert them to Error.

In short, std::error_code makes for a simpler, less risky change that fits with the existing code and semantics, so I chose to go with that.

I totally agree. :)

Looks good!

llvm/lib/LTO/LTOCodeGenerator.cpp

263 ↗

(On Diff #119924)

Separately, emitError and emitWarning should really take something other than std::string.

[rnk]

Oh, do you think this is worth a unit test? It should be easy to add one for "file not found" or something, if you try creating a file in a directory that doesn't exist.

[inglorion]

Oh, do you think this is worth a unit test? It should be easy to add one for "file not found" or something, if you try creating a file in a directory that doesn't exist.

I would like to have a test for this, but after trying a few things for a couple of hours I decided to punt on that and try to ship the improvement without adding a test. The issue is that this change only applies to errors that happen after the file has been opened (errors during open were already reported back to the caller). That makes it harder to come up with a test that works reliably across operating systems. I tried opening the file and getting an fd, then passing the fd to the raw_fd_ostream constructor that takes one, closing the fd, then writing to the stream - but the result is that the test dies before ever reporting success or failure (on Windows). There are other things we could do, for example mocking the calls that actually interact with the filesystem, but that didn't really seem worth the effort. If you agree, I'd like to ship this without the test for now.

[rnk]

In D39203#904284, @inglorion wrote:

If you agree, I'd like to ship this without the test for now.

Yep, let's land this. Writing a portable test for write/WriteFile errors is hard.

llvm/trunk/include/llvm/Support/LockFileManager.h

public:
   /// \brief Remove the lock file.  This may delete a different lock file than
   /// the one previously read if there is a race.
   std::error_code unsafeRemoveLockFile();
 
   /// \brief Get error message, or "" if there is no error.
   std::string getErrorMessage() const;
 
   /// \brief Set error and error message
-  void setError(std::error_code &EC, StringRef ErrorMsg = "") {
+  void setError(const std::error_code &EC, StringRef ErrorMsg = "") {
     Error = EC;
     ErrorDiagMsg = ErrorMsg.str();
   }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_LOCKFILEMANAGER_H

llvm/trunk/include/llvm/Support/raw_ostream.h

 //===----------------------------------------------------------------------===//
 
 /// A raw_ostream that writes to a file descriptor.
 ///
 class raw_fd_ostream : public raw_pwrite_stream {
   int FD;
   bool ShouldClose;
 
-  /// Error This flag is true if an error of any kind has been detected.
-  ///
-  bool Error;
+  std::error_code EC;
 
   uint64_t pos;
 
   bool SupportsSeeking;
 
   /// See raw_ostream::write_impl.
   void write_impl(const char *Ptr, size_t Size) override;
 
   void pwrite_impl(const char *Ptr, size_t Size, uint64_t Offset) override;
 
   /// Return the current position within the stream, not counting the bytes
   /// currently in the buffer.
   uint64_t current_pos() const override { return pos; }
 
   /// Determine an efficient buffer size.
   size_t preferred_buffer_size() const override;
 
   /// Set the flag indicating that an output error has been encountered.
-  void error_detected() { Error = true; }
+  void error_detected(std::error_code EC) { this->EC = EC; }
 
 public:
   /// Open the specified file for writing. If an error occurs, information
   /// about the error is put into EC, and the stream should be immediately
   /// destroyed;
   /// \p Flags allows optional flags to control how the file will be opened.
   ///
   /// As a special case, if Filename is "-", then the stream will use
public:
   raw_ostream &resetColor() override;
 
   raw_ostream &reverseColor() override;
 
   bool is_displayed() const override;
 
   bool has_colors() const override;
 
+  std::error_code error() const { return EC; }
+
   /// Return the value of the flag in this raw_fd_ostream indicating whether an
   /// output error has been encountered.
   /// This doesn't implicitly flush any pending output.  Also, it doesn't
   /// guarantee to detect all errors unless the stream has been closed.
-  bool has_error() const {
-    return Error;
-  }
+  bool has_error() const { return bool(EC); }
 
   /// Set the flag read by has_error() to false. If the error flag is set at the
   /// time when this raw_ostream's destructor is called, report_fatal_error is
   /// called to report the error. Use clear_error() after handling the error to
   /// avoid this behavior.
   ///
   ///   "Errors should never pass silently.
   ///    Unless explicitly silenced."
   ///      - from The Zen of Python, by Tim Peters
   ///
-  void clear_error() {
-    Error = false;
-  }
+  void clear_error() { EC = std::error_code(); }
 };
 
 /// This returns a reference to a raw_ostream for standard output. Use it like:
 /// outs() << "foo" << "bar";
 raw_ostream &outs();
 
 /// This returns a reference to a raw_ostream for standard error. Use it like:
 /// errs() << "foo" << "bar";

llvm/trunk/lib/IR/Core.cpp

if (EC) {
     return true;
   }
 
   unwrap(M)->print(dest, nullptr);
 
   dest.close();
 
   if (dest.has_error()) {
-    *ErrorMessage = strdup("Error printing to file");
+    std::string E = "Error printing to file: " + dest.error().message();
+    *ErrorMessage = strdup(E.c_str());
     return true;
   }
 
   return false;
 }
 
 char *LLVMPrintModuleToString(LLVMModuleRef M) {
   std::string buf;

llvm/trunk/lib/LTO/LTOCodeGenerator.cpp

bool LTOCodeGenerator::writeMergedModules(StringRef Path) {
   // mark which symbols can not be internalized
   applyScopeRestrictions();
 
   // create output file
   std::error_code EC;
   ToolOutputFile Out(Path, EC, sys::fs::F_None);
   if (EC) {
     std::string ErrMsg = "could not open bitcode file for writing: ";
-    ErrMsg += Path;
+    ErrMsg += Path.str() + ": " + EC.message();
     emitError(ErrMsg);
     return false;
   }
 
   // write bitcode to it
   WriteBitcodeToFile(MergedModule.get(), Out.os(), ShouldEmbedUselists);
   Out.os().close();
 
   if (Out.os().has_error()) {
     std::string ErrMsg = "could not write bitcode file: ";
-    ErrMsg += Path;
+    ErrMsg += Path.str() + ": " + Out.os().error().message();
     emitError(ErrMsg);
     Out.os().clear_error();
     return false;
   }
 
   Out.keep();
   return true;
 }
bool LTOCodeGenerator::compileOptimizedToFile(const char **Name) {
   }
 
   // generate object file
   ToolOutputFile objFile(Filename, FD);
 
   bool genResult = compileOptimized(&objFile.os());
   objFile.os().close();
   if (objFile.os().has_error()) {
-    emitError((Twine("could not write object file: ") + Filename).str());
+    emitError((Twine("could not write object file: ") + Filename + ": " +
+               objFile.os().error().message())
+                  .str());
     objFile.os().clear_error();
     sys::fs::remove(Twine(Filename));
     return false;
   }
 
   objFile.keep();
   if (!genResult) {
     sys::fs::remove(Twine(Filename));

llvm/trunk/lib/Support/LockFileManager.cpp

 #if LLVM_ON_UNIX
     Out << getpid();
 #else
     Out << "1";
 #endif
     Out.close();
 
     if (Out.has_error()) {
-      // We failed to write out PID, so make up an excuse, remove the
+      // We failed to write out PID, so report the error, remove the
       // unique lock file, and fail.
-      auto EC = make_error_code(errc::no_space_on_device);
       std::string S("failed to write to ");
       S.append(UniqueLockFileName.str());
-      setError(EC, S);
+      setError(Out.error(), S);
       sys::fs::remove(UniqueLockFileName);
       return;
     }
   }
 
   // Clean up the unique file on signal, which also releases the lock if it is
   // held since the .lock symlink will point to a nonexistent file.
   RemoveUniqueLockFileOnSignal RemoveUniqueFile(UniqueLockFileName);

llvm/trunk/lib/Support/raw_ostream.cpp

 
 raw_fd_ostream::raw_fd_ostream(StringRef Filename, std::error_code &EC,
                                sys::fs::OpenFlags Flags)
     : raw_fd_ostream(getFD(Filename, EC, Flags), true) {}
 
 /// FD is the file descriptor that this writes to.  If ShouldClose is true, this
 /// closes the file when the stream is destroyed.
 raw_fd_ostream::raw_fd_ostream(int fd, bool shouldClose, bool unbuffered)
-    : raw_pwrite_stream(unbuffered), FD(fd), ShouldClose(shouldClose),
-      Error(false) {
+    : raw_pwrite_stream(unbuffered), FD(fd), ShouldClose(shouldClose) {
   if (FD < 0 ) {
     ShouldClose = false;
     return;
   }
 
   // Do not attempt to close stdout or stderr. We used to try to maintain the
   // property that tools that support writing file to stdout should not also
   // write informational output to stdout, but in practice we were never able to
if (!SupportsSeeking)
     pos = 0;
   else
     pos = static_cast<uint64_t>(loc);
 }
 
 raw_fd_ostream::~raw_fd_ostream() {
   if (FD >= 0) {
     flush();
-    if (ShouldClose && sys::Process::SafelyCloseFileDescriptor(FD))
-      error_detected();
+    if (ShouldClose) {
+      if (auto EC = sys::Process::SafelyCloseFileDescriptor(FD))
+        error_detected(EC);
+    }
   }
 
 #ifdef __MINGW32__
   // On mingw, global dtors should not call exit().
   // report_fatal_error() invokes exit(). We know report_fatal_error()
   // might not write messages to stderr when any errors were detected
   // on FD == 2.
   if (FD == 2) return;
 #endif
 
   // If there are any pending errors, report them now. Clients wishing
   // to avoid report_fatal_error calls should check for errors with
   // has_error() and clear the error flag with clear_error() before
   // destructing raw_ostream objects which may have errors.
   if (has_error())
-    report_fatal_error("IO failure on output stream.", /*GenCrashDiag=*/false);
+    report_fatal_error("IO failure on output stream: " + error().message(),
+                       /*GenCrashDiag=*/false);
 }
 
 void raw_fd_ostream::write_impl(const char *Ptr, size_t Size) {
   assert(FD >= 0 && "File already closed.");
   pos += Size;
 
 #ifndef LLVM_ON_WIN32
 #if defined(__linux__)
if (ret < 0) {
       if (errno == EINTR || errno == EAGAIN
 #ifdef EWOULDBLOCK
           || errno == EWOULDBLOCK
 #endif
           )
         continue;
 
       // Otherwise it's a non-recoverable error. Note it and quit.
-      error_detected();
+      error_detected(std::error_code(errno, std::generic_category()));
       break;
     }
 
     // The write may have written some or all of the data. Update the
     // size and buffer pointer to reflect the remainder that needs
     // to be written. If there are no bytes left, we're done.
     Ptr += ret;
     Size -= ret;
   } while (Size > 0);
 }
 
 void raw_fd_ostream::close() {
   assert(ShouldClose);
   ShouldClose = false;
   flush();
-  if (sys::Process::SafelyCloseFileDescriptor(FD))
-    error_detected();
+  if (auto EC = sys::Process::SafelyCloseFileDescriptor(FD))
+    error_detected(EC);
   FD = -1;
 }
 
 uint64_t raw_fd_ostream::seek(uint64_t off) {
   assert(SupportsSeeking && "Stream does not support seeking!");
   flush();
 #ifdef LLVM_ON_WIN32
   pos = ::_lseeki64(FD, off, SEEK_SET);
 #elif defined(HAVE_LSEEK64)
   pos = ::lseek64(FD, off, SEEK_SET);
 #else
   pos = ::lseek(FD, off, SEEK_SET);
 #endif
   if (pos == (uint64_t)-1)
-    error_detected();
+    error_detected(std::error_code(errno, std::generic_category()));
   return pos;
 }
 
 void raw_fd_ostream::pwrite_impl(const char *Ptr, size_t Size,
                                  uint64_t Offset) {
   uint64_t Pos = tell();
   seek(Offset);
   write(Ptr, Size);