Add raw_fd_stream that supports reading/seeking/writing

[stephan.yichao.zhao]

Summary

This is used by https://reviews.llvm.org/D86905 to support bitcode writer's incremental flush.

[stephan.yichao.zhao]

update

[stephan.yichao.zhao]

update

[JDevlieghere]

Other than a small style nit this LGTM

llvm/lib/Support/raw_ostream.cpp

919

nit: the llvm style guide says no braces around single-line statements. Same in raw_fd_stream::read below.

[MaskRay]

llvm/lib/Support/raw_ostream.cpp

919

If stdout happens to be seekable, can the Filename == "-" restriction be lifted?

926

Ret

927

no braces

[stephan.yichao.zhao]

llvm/lib/Support/raw_ostream.cpp

919

Thanks. Just learned that stdout is seekable if it is redirected to a normal file.

[stephan.yichao.zhao]

addressed comments

[jhenderson]

llvm/include/llvm/Support/raw_ostream.h

591–592

This comment doesn't seem to line up with the interface, where it talks about setting EC on failure. I'm guessing this isn't the EC from the constructor, since there's no state involved, and theres' no other EC parameter!

llvm/unittests/Support/raw_fd_stream_test.cpp

2

A couple of other test cases I'd like to see, depending on how practical it is (I suspect it's not practical, and if so, don't worry).

1. An unseekable input (i.e. where supportsSeeking returns false).
2. An unseekable input, but where EC was already in an error state (and so the new error wasn't reported).

39

Is this environment variable ever going to be set? I'm not sure what the purpose of the extra logic is...

44

I'm not sure I understand the need for the macro. Can this not just be the following?

ASSERT_FALSE(sys::fs::createTemporaryFile("foo", "bar", FD, PATH));

(with optional explicit operator bool call, if needed)

48

ec -> EC for LLVM style.

78

Ditto ec -> EC here too.

[stephan.yichao.zhao]

update

[stephan.yichao.zhao]

llvm/include/llvm/Support/raw_ostream.h

591–592

Updated comments.

llvm/unittests/Support/raw_fd_stream_test.cpp

2

Looked into gmock a bit. To mock supportsSeeking, the code under test needs to rewrite to use either virtual method or templete: https://github.com/google/googletest/blob/master/googlemock/docs/cook_book.md#MockingNonVirtualMethods

Another way to test it is like this. At raw_fd_ostream::raw_fd_ostream, at a place where SupportsSeeking is set, we do

if GlobalMockValue is set:
  SupportsSeeking =  GlobalMockValue
else
  SupportsSeeking = ...

Here GlobalMockValue could be a global pointer. And the unit test can set it. EC can be tested in the same way.

But this also needs to change the code under test. Did any other LLVM test code do something similar?

39

This code refers to the test raw_pwrite_ostreamTest from llvm/unittests/Support/raw_pwrite_stream_test.cpp

That test has a death test at the end. This new test does not have death test. I removed unrelated code.

[jhenderson]

LGTM.

llvm/unittests/Support/raw_fd_stream_test.cpp

2

Yeah, I didn't think this through hard enough, and I don't see a great way of doing this in the tests (I don't think rewriting the code for greater testability is necessary). If you can manually locally verify that the code works as desired, then that would be sufficient for my comment.

[MaskRay]

Looks great!

llvm/include/llvm/Support/raw_ostream.h

55

OK_LastFDStream is not used and should be deleted

[stephan.yichao.zhao]

addressed comments

[stephan.yichao.zhao]

addressed comments

[stephan.yichao.zhao]

submitted by https://github.com/llvm/llvm-project/commit/0ece51c60c51f0d4c285dbda3b6cff794041bdd7 and https://github.com/llvm/llvm-project/commit/19531a81f1de8ef8ee219765c74c32c6fcd3323f

llvm/include/llvm/Support/raw_ostream.h

 } // end namespace fs
 } // end namespace sys
 
 /// This class implements an extremely fast bulk output stream that can *only*
 /// output to a stream.  It does not support seeking, reopening, rewinding, line
 /// buffered disciplines etc. It is a simple buffer that outputs
 /// a chunk at a time.
 class raw_ostream {
+public:
+  // Class kinds to support LLVM-style RTTI.
+  enum class OStreamKind {
+    OK_OStream,
+    OK_FDStream,
+  };
+
 private:
+  OStreamKind Kind;
+
   /// The buffer is handled in such a way that the buffer is
   /// uninitialized, unbuffered, or out of space when OutBufCur >=
   /// OutBufEnd. Thus a single comparison suffices to determine if we
   /// need to take the slow path to write a single character.
   ///
   /// The buffer is in one of three states:
   ///  1. Unbuffered (BufferMode == Unbuffered)
   ///  1. Uninitialized (BufferMode != Unbuffered && OutBufStart == 0).
public:
   static constexpr Colors YELLOW = Colors::YELLOW;
   static constexpr Colors BLUE = Colors::BLUE;
   static constexpr Colors MAGENTA = Colors::MAGENTA;
   static constexpr Colors CYAN = Colors::CYAN;
   static constexpr Colors WHITE = Colors::WHITE;
   static constexpr Colors SAVEDCOLOR = Colors::SAVEDCOLOR;
   static constexpr Colors RESET = Colors::RESET;
 
-  explicit raw_ostream(bool unbuffered = false)
-      : BufferMode(unbuffered ? BufferKind::Unbuffered
+  explicit raw_ostream(bool unbuffered = false,
+                       OStreamKind K = OStreamKind::OK_OStream)
+      : Kind(K), BufferMode(unbuffered ? BufferKind::Unbuffered
                                        : BufferKind::InternalBuffer) {
     // Start out ready to flush.
     OutBufStart = OutBufEnd = OutBufCur = nullptr;
   }
 
   raw_ostream(const raw_ostream &) = delete;
   void operator=(const raw_ostream &) = delete;
 
   virtual ~raw_ostream();
 
   /// tell - Return the current offset with the file.
   uint64_t tell() const { return current_pos() + GetNumBytesInBuffer(); }
 
+  OStreamKind get_kind() const { return Kind; }
+
   //===--------------------------------------------------------------------===//
   // Configuration Interface
   //===--------------------------------------------------------------------===//
 
   /// Set the stream to be buffered, with an automatically determined buffer
   /// size.
   void SetBuffered();
 

 /// An abstract base class for streams implementations that also support a
 /// pwrite operation. This is useful for code that can mostly stream out data,
 /// but needs to patch in a header that needs to know the output size.
 class raw_pwrite_stream : public raw_ostream {
   virtual void pwrite_impl(const char *Ptr, size_t Size, uint64_t Offset) = 0;
   void anchor() override;
 
 public:
-  explicit raw_pwrite_stream(bool Unbuffered = false)
-      : raw_ostream(Unbuffered) {}
+  explicit raw_pwrite_stream(bool Unbuffered = false,
+                             OStreamKind K = OStreamKind::OK_OStream)
+      : raw_ostream(Unbuffered, K) {}
   void pwrite(const char *Ptr, size_t Size, uint64_t Offset) {
 #ifndef NDEBUG
     uint64_t Pos = tell();
     // /dev/null always reports a pos of 0, so we cannot perform this check
     // in that case.
     if (Pos)
       assert(Size + Offset <= Pos && "We don't support extending the stream");
 #endif
#endif
 
   /// 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;
 
+  void anchor() override;
+
+protected:
   /// Set the flag indicating that an output error has been encountered.
   void error_detected(std::error_code EC) { this->EC = EC; }
 
-  void anchor() override;
+  /// Return the file descriptor.
+  int get_fd() const { return FD; }
+
+  // Update the file position by increasing \p Delta.
+  void inc_pos(uint64_t Delta) { pos += Delta; }
 
 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
   /// STDOUT_FILENO instead of opening a file. This will not close the stdout
   /// descriptor.
   raw_fd_ostream(StringRef Filename, std::error_code &EC);
   raw_fd_ostream(StringRef Filename, std::error_code &EC,
                  sys::fs::CreationDisposition Disp);
   raw_fd_ostream(StringRef Filename, std::error_code &EC,
                  sys::fs::FileAccess Access);
   raw_fd_ostream(StringRef Filename, std::error_code &EC,
                  sys::fs::OpenFlags Flags);
   raw_fd_ostream(StringRef Filename, std::error_code &EC,
                  sys::fs::CreationDisposition Disp, sys::fs::FileAccess Access,
                  sys::fs::OpenFlags Flags);
 
   /// FD is the file descriptor that this writes to.  If ShouldClose is true,
   /// this closes the file when the stream is destroyed. If FD is for stdout or
   /// stderr, it will not be closed.
-  raw_fd_ostream(int fd, bool shouldClose, bool unbuffered=false);
+  raw_fd_ostream(int fd, bool shouldClose, bool unbuffered = false,
+                 OStreamKind K = OStreamKind::OK_OStream);
 
   ~raw_fd_ostream() override;
 
   /// Manually flush the stream and close the file. Note that this does not call
   /// fsync.
   void close();
 
   bool supportsSeeking() const { return SupportsSeeking; }

 /// stderr is written, to ensure the error messages are written in their
 /// expected place.
 raw_fd_ostream &errs();
 
 /// This returns a reference to a raw_ostream which simply discards output.
 raw_ostream &nulls();
 
 //===----------------------------------------------------------------------===//
+// File Streams
+//===----------------------------------------------------------------------===//
+
+/// A raw_ostream of a file for reading/writing/seeking.
+///
+class raw_fd_stream : public raw_fd_ostream {
+public:
+  /// Open the specified file for reading/writing/seeking. If an error occurs,
+  /// information about the error is put into EC, and the stream should be
+  /// immediately destroyed.
+  raw_fd_stream(StringRef Filename, std::error_code &EC);
+
+  /// This reads the \p Size bytes into a buffer pointed by \p Ptr.
+  ///
+  /// \param Ptr The start of the buffer to hold data to be read.
+  ///
+  /// \param Size The number of bytes to be read.
+  ///
+  /// On success, the number of bytes read is returned, and the file position is
+  /// advanced by this number. On error, -1 is returned, use error() to get the
+  /// error code.
+  ssize_t read(char *Ptr, size_t Size);
+
+  /// Check if \p OS is a pointer of type raw_fd_stream*.
+  static bool classof(const raw_ostream *OS);
+};
+
+//===----------------------------------------------------------------------===//
 // Output Stream Adaptors
 //===----------------------------------------------------------------------===//
 
 /// A raw_ostream that writes to an std::string.  This is a simple adaptor
 /// class. This class does not encounter output errors.
 class raw_string_ostream : public raw_ostream {
   std::string &OS;
 

llvm/lib/Support/raw_ostream.cpp

 raw_fd_ostream::raw_fd_ostream(StringRef Filename, std::error_code &EC,
                                sys::fs::CreationDisposition Disp,
                                sys::fs::FileAccess Access,
                                sys::fs::OpenFlags Flags)
     : raw_fd_ostream(getFD(Filename, EC, Disp, Access, 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) {
+raw_fd_ostream::raw_fd_ostream(int fd, bool shouldClose, bool unbuffered,
+                               OStreamKind K)
+    : raw_pwrite_stream(unbuffered, K), FD(fd), ShouldClose(shouldClose) {
   if (FD < 0 ) {
     ShouldClose = false;
     return;
   }
 
   enable_colors(true);
 
   // Do not attempt to close stdout or stderr. We used to try to maintain the

 
 /// nulls() - This returns a reference to a raw_ostream which discards output.
 raw_ostream &llvm::nulls() {
   static raw_null_ostream S;
   return S;
 }
 
 //===----------------------------------------------------------------------===//
+// File Streams
+//===----------------------------------------------------------------------===//
+
+raw_fd_stream::raw_fd_stream(StringRef Filename, std::error_code &EC)
+    : raw_fd_ostream(getFD(Filename, EC, sys::fs::CD_CreateAlways,
+                           sys::fs::FA_Write | sys::fs::FA_Read,
+                           sys::fs::OF_None),
+                     true, false, OStreamKind::OK_FDStream) {
+  if (EC)
+    return;
+
+  // Do not support non-seekable files.
+  if (!supportsSeeking())
+    EC = std::make_error_code(std::errc::invalid_argument);
+}
+
+ssize_t raw_fd_stream::read(char *Ptr, size_t Size) {
+  assert(get_fd() >= 0 && "File already closed.");
+  ssize_t Ret = ::read(get_fd(), (void *)Ptr, Size);
+  if (Ret >= 0)
+    inc_pos(Ret);
+  else
+    error_detected(std::error_code(errno, std::generic_category()));
+  return Ret;
+}
+
+bool raw_fd_stream::classof(const raw_ostream *OS) {
+  return OS->get_kind() == OStreamKind::OK_FDStream;
+}
+
+//===----------------------------------------------------------------------===//
 //  raw_string_ostream
 //===----------------------------------------------------------------------===//
 
 raw_string_ostream::~raw_string_ostream() {
   flush();
 }
 
 void raw_string_ostream::write_impl(const char *Ptr, size_t Size) {

llvm/unittests/Support/raw_fd_stream_test.cpp

+//===- llvm/unittest/Support/raw_fd_stream_test.cpp - raw_fd_stream 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/ADT/SmallString.h"
+#include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FileUtilities.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+
+TEST(raw_fd_streamTest, ReadAfterWrite) {
+  SmallString<64> Path;
+  int FD;
+  ASSERT_FALSE(sys::fs::createTemporaryFile("foo", "bar", FD, Path));
+  FileRemover Cleanup(Path);
+  std::error_code EC;
+  raw_fd_stream OS(Path, EC);
+  EXPECT_TRUE(!EC);
+
+  char Bytes[8];
+
+  OS.write("01234567", 8);
+
+  OS.seek(3);
+  EXPECT_EQ(OS.read(Bytes, 2), 2);
+  EXPECT_EQ(Bytes[0], '3');
+  EXPECT_EQ(Bytes[1], '4');
+
+  OS.seek(4);
+  OS.write("xyz", 3);
+
+  OS.seek(0);
+  EXPECT_EQ(OS.read(Bytes, 8), 8);
+  EXPECT_EQ(Bytes[0], '0');
+  EXPECT_EQ(Bytes[1], '1');
+  EXPECT_EQ(Bytes[2], '2');
+  EXPECT_EQ(Bytes[3], '3');
+  EXPECT_EQ(Bytes[4], 'x');
+  EXPECT_EQ(Bytes[5], 'y');
+  EXPECT_EQ(Bytes[6], 'z');
+  EXPECT_EQ(Bytes[7], '7');
+}
+
+TEST(raw_fd_streamTest, DynCast) {
+  {
+    std::error_code EC;
+    raw_fd_stream OS("-", EC);
+    EXPECT_TRUE(dyn_cast<raw_fd_stream>(&OS));
+  }
+  {
+    std::error_code EC;
+    raw_fd_ostream OS("-", EC);
+    EXPECT_FALSE(dyn_cast<raw_fd_stream>(&OS));
+  }
+}
+
+} // namespace