[llvm-objcopy][Support] move writeToStream helper function to Support.

[avl]

Summary

writeToStream function is useful when it is necessary to create different kinds
of streams(based on stream name) and when we need to use a temporary file
while writing(which would be renamed into the resulting file in a success case).
This patch moves the writeToStream helper into the Support library.

[dblaikie]

Sounds good to me

[avl]

Thank you for the review!

[jhenderson]

llvm/include/llvm/Support/raw_ostream.h

719–724

There are a number of issues with the original comment, which we can fix whilst you're moving it. Please then reflow the comment to the 80 character limit. I don't understand what the last sentence of the comment is trying to say. I've suggested an alternative, but I'm not sure if it is right or not.

It seems to me like this should actually be called writeToOutput, not writeToStream. It is using the stream to write to some output, whilst the writing to the stream is actually inside the Write argument. I'm also not convinced that this belongs in the raw_ostream file, since it's really about writing to files (I think it maybe belongs in some File I/O file within the Support library), but I'm less concerned by that, since it is using a raw_ostream.

llvm/unittests/Support/raw_ostream_test.cpp

493–500

I believe you actually want this approach. Similar applies to each of the others below.

532–540

This should be as suggested inline.

544

Use EXPECT_THAT_ERROR(..., Succeeded()) here and below too.

545

Perhaps add something in this functor to show that it is actually run, and then check that thing after the call has finished.

554

I'm not sure we should be writing to stdout directly here, unless you've got some way of capturing and redirecting stdout back so that you can check it. In fact, as things stand, the writeToStream function could just return Error::success() without even calling the underlying functor. You need to a) show that the raw_ostream in use is the stdout stream, and b) show that the functor is called.

[avl]

addressed comments.

I did not do two changes yet:

• renaming writeToStream into writeToOutput.
• moving writeToStream/writeToOutput into the other file.

It seems to me that writeToStream clearly shows that we write into the stream.
That is true that real writing happens inside Write argument. But there is no any
possibility to write into other output than subclass of raw_ostream. So,
when we call writeToStream we always write into the some stream.

It also seems natural to have writeToStream into the raw_ostream.h/cpp since this
function is for streams which are subclusses of raw_ostream. And all these subclusses
are defined in raw_ostream.h/cpp.

If you still think that above changes(rename writeToStream into writeToOutput, move
writeToStream/writeToOutput into other file) should be done I would do this in next update.

[jhenderson]

In D98426#2635865, @avl wrote:

addressed comments.

I did not do two changes yet:

• renaming writeToStream into writeToOutput.
• moving writeToStream/writeToOutput into the other file.

It seems to me that writeToStream clearly shows that we write into the stream.
That is true that real writing happens inside Write argument. But there is no any
possibility to write into other output than subclass of raw_ostream. So,
when we call writeToStream we always write into the some stream.

It also seems natural to have writeToStream into the raw_ostream.h/cpp since this
function is for streams which are subclusses of raw_ostream. And all these subclusses
are defined in raw_ostream.h/cpp.

If you still think that above changes(rename writeToStream into writeToOutput, move
writeToStream/writeToOutput into other file) should be done I would do this in next update.

I'm not too fussed about which file, as I don't have a good alternative location for it. I do think we should do the function renaming however. When I see writeToStream, I expect to pass in a stream to write to, but the function signature doesn't take a stream. writeToOutput meanwhile indicates the function will write to some specified output, which is a higher level thing.

There is of course a completely different approach, which would avoid this issue, namely to change the function to just create and return the stream and then leave the clients to pass the stream to their write functions. If you decided to switch to this approach, you'd probably want a new stream class derived from raw_fd_ostream which does the tempfile stuff itself, in the constructor/destructor, so that clients don't need t worry about that logic.

llvm/unittests/Support/raw_ostream_test.cpp

509

Hmmm, on further reflection, I bet this will fail on some systems, because the spelling/capitalization etc of this message is dependent on the c++ standard library implementation (see various recent attempts to solve this problem in lit). You might need to do something like get the expected message from a local instantiation of the std::errc that matches and use that to compute the expected output.

513

518

You could probably delete this line, since it doesn't actually do anything.

526

You can use EXPECT_TRUE here, because this value being incorrect doesn't prevent the rest of the test from running (it's not really necessary, because this is the last statement, but it's a good habit to get into - use ASSERT... if and only if later statements rely on the asserted statement being true, e.g. ASSERT_TRUE that a pointer is not null, before dereferencing it in a later check.

[avl]

There is of course a completely different approach, which would avoid this issue, namely to change the function to just create and return the stream and then leave the clients to pass the stream to their write functions. If you decided to switch to this approach, you'd probably want a new stream class derived from raw_fd_ostream which does the tempfile stuff itself, in the constructor/destructor, so that clients don't need t worry about that logic.

I thought about similar approach:

std::unique_ptr<raw_ostream> Stream = createStream(FileName);

Stream << "data to write";

But it makes error handling to be inconvenient. The subclass of raw_fd_ostream needs to return Error from it`s destructor.

raw_fd_ostream_sub::~raw_fd_ostream_sub () {
    Error Err = Temp->keep(OutputFileName);
}

So it is not clear how to get that error from the destructor of std::unique_ptr(which calls destructor of subclass of raw_fd_ostream).

[avl]

addressed comments.

[jhenderson]

LGTM, thanks!

llvm/unittests/Support/raw_ostream_test.cpp

487

[avl]

Thank you, for the review.

[hokein]

llvm/lib/Support/raw_ostream.cpp

1003

is the all_exe mode intended here? (the clang-include-cleaner tool is using this API, I was debugging an issue where the file type of the modified file was changed executable).

[avl]

llvm/lib/Support/raw_ostream.cpp

1003

yep, It looks incorrectly to set it by default. It would be better to remove it or make configurable.

there is similar API llvm::writeFileAtomically. probably it would be better to unite both of this APIs.

[hokein]

llvm/lib/Support/raw_ostream.cpp

1003

yep, It looks incorrectly to set it by default. It would be better to remove it or make configurable.

thanks. I will sent out a patch.

there is similar API llvm::writeFileAtomically. probably it would be better to unite both of this APIs.

Sounds good to me. There is only 2 in-tree usages of llvm::writeFileAtomically API, I think we can even just remove this API.

llvm/include/llvm/Support/raw_ostream.h

class buffer_unique_ostream : public raw_svector_ostream {
   virtual void anchor() override;
 
 public:
   buffer_unique_ostream(std::unique_ptr<raw_ostream> OS)
       : raw_svector_ostream(Buffer), OS(std::move(OS)) {}
   ~buffer_unique_ostream() override { *OS << str(); }
 };
 
+class Error;
+
+/// This helper creates an output stream and then passes it to \p Write.
+/// The stream created is based on the specified \p OutputFileName:
+/// llvm::outs for "-", raw_null_ostream for "/dev/null", and raw_fd_ostream
+/// for other names. For raw_fd_ostream instances, the stream writes to
+/// a temporary file. The final output file is atomically replaced with the
+/// temporary file after the \p Write function is finished.
+Error writeToOutput(StringRef OutputFileName,
+                    std::function<Error(raw_ostream &)> Write);
+
 } // end namespace llvm
 
 #endif // LLVM_SUPPORT_RAW_OSTREAM_H

llvm/lib/Support/raw_ostream.cpp

 void raw_null_ostream::pwrite_impl(const char *Ptr, size_t Size,
                                    uint64_t Offset) {}
 
 void raw_pwrite_stream::anchor() {}
 
 void buffer_ostream::anchor() {}
 
 void buffer_unique_ostream::anchor() {}
+
+Error llvm::writeToOutput(StringRef OutputFileName,
+                          std::function<Error(raw_ostream &)> Write) {
+  if (OutputFileName == "-")
+    return Write(outs());
+
+  if (OutputFileName == "/dev/null") {
+    raw_null_ostream Out;
+    return Write(Out);
+  }
+
+  unsigned Mode = sys::fs::all_read | sys::fs::all_write | sys::fs::all_exe;
+  Expected<sys::fs::TempFile> Temp =
+      sys::fs::TempFile::create(OutputFileName + ".temp-stream-%%%%%%", Mode);
+  if (!Temp)
+    return createFileError(OutputFileName, Temp.takeError());
+
+  raw_fd_ostream Out(Temp->FD, false);
+
+  if (Error E = Write(Out)) {
+    if (Error DiscardError = Temp->discard())
+      return joinErrors(std::move(E), std::move(DiscardError));
+    return E;
+  }
+  Out.flush();
+
+  return Temp->keep(OutputFileName);
+}

llvm/tools/llvm-objcopy/llvm-objcopy.h

 
 } // end namespace object
 
 namespace objcopy {
 struct CopyConfig;
 Expected<std::vector<NewArchiveMember>>
 createNewArchiveMembers(CopyConfig &Config, const object::Archive &Ar);
 
-/// A writeToFile helper creates an output stream, based on the specified
-/// \p OutputFileName: std::outs for the "-", raw_null_ostream for
-/// the "/dev/null", temporary file in the same directory as the final output
-/// file for other names. The final output file is atomically replaced with
-/// the temporary file after \p Write handler is finished.
-Error writeToFile(StringRef OutputFileName,
-                  std::function<Error(raw_ostream &)> Write);
-
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_OBJCOPY_H

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

 #include <memory>
 #include <string>
 #include <system_error>
 #include <utility>
 
 namespace llvm {
 namespace objcopy {
 
-Error writeToFile(StringRef OutputFileName,
-                  std::function<Error(raw_ostream &)> Write) {
-  if (OutputFileName == "-")
-    return Write(outs());
-
-  if (OutputFileName == "/dev/null") {
-    raw_null_ostream Out;
-    return Write(Out);
-  }
-
-  unsigned Mode = sys::fs::all_read | sys::fs::all_write | sys::fs::all_exe;
-  Expected<sys::fs::TempFile> Temp =
-      sys::fs::TempFile::create(OutputFileName + ".temp-objcopy-%%%%%%", Mode);
-  if (!Temp)
-    return createFileError(OutputFileName, Temp.takeError());
-
-  raw_fd_ostream Out(Temp->FD, false);
-
-  if (Error E = Write(Out)) {
-    if (Error DiscardError = Temp->discard())
-      return joinErrors(std::move(E), std::move(DiscardError));
-    return E;
-  }
-  Out.flush();
-
-  return Temp->keep(OutputFileName);
-}
-
 // The name this program was invoked as.
 StringRef ToolName;
 
 ErrorSuccess reportWarning(Error E) {
   assert(E);
   WithColor::warning(errs(), ToolName) << toString(std::move(E)) << '\n';
   return Error::success();
 }
if (Archive *Ar = dyn_cast<Archive>(BinaryHolder.getBinary())) {
       };
     }
   }
 
   if (ObjcopyFunc) {
     if (Config.SplitDWO.empty()) {
       // Apply transformations described by Config and store result into
       // Config.OutputFilename using specified ObjcopyFunc function.
-      if (Error E = writeToFile(Config.OutputFilename, ObjcopyFunc))
+      if (Error E = writeToOutput(Config.OutputFilename, ObjcopyFunc))
         return E;
     } else {
       Config.ExtractDWO = true;
       Config.StripDWO = false;
       // Copy .dwo tables from the Config.InputFilename into Config.SplitDWO
       // file using specified ObjcopyFunc function.
-      if (Error E = writeToFile(Config.SplitDWO, ObjcopyFunc))
+      if (Error E = writeToOutput(Config.SplitDWO, ObjcopyFunc))
         return E;
       Config.ExtractDWO = false;
       Config.StripDWO = true;
       // Apply transformations described by Config, remove .dwo tables and
       // store result into Config.OutputFilename using specified ObjcopyFunc
       // function.
-      if (Error E = writeToFile(Config.OutputFilename, ObjcopyFunc))
+      if (Error E = writeToOutput(Config.OutputFilename, ObjcopyFunc))
         return E;
     }
   }
 
   if (Error E = restoreStatOnFile(Config.OutputFilename, Stat, Config))
     return E;
 
   if (!Config.SplitDWO.empty()) {

llvm/unittests/Support/raw_ostream_test.cpp

 //===- llvm/unittest/Support/raw_ostream_test.cpp - raw_ostream 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/Support/FileSystem.h"
+#include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/Format.h"
+#include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace {
 
 template<typename T> std::string printToString(const T &Value) {
   std::string res;
TEST(raw_ostreamTest, reserve_stream) {
   OS.reserveExtraSpace(1000);
   EXPECT_TRUE(Str.capacity() >= CurrentPos + 1000);
   OS << "hello";
   OS << 1;
   OS << 'w' << 'o' << 'r' << 'l' << 'd';
   OS.flush();
   EXPECT_EQ("11111111111111111111hello1world", Str);
 }
+
+static void checkFileData(StringRef FileName, StringRef GoldenData) {
+  ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
+      MemoryBuffer::getFileOrSTDIN(FileName);
+  EXPECT_FALSE(BufOrErr.getError());
+
+  EXPECT_EQ((*BufOrErr)->getBufferSize(), GoldenData.size());
+  EXPECT_EQ(memcmp((*BufOrErr)->getBufferStart(), GoldenData.data(),
+                   GoldenData.size()),
+            0);
+}
+
+TEST(raw_ostreamTest, writeToOutputFile) {
+  SmallString<64> Path;
+  int FD;
+  ASSERT_FALSE(sys::fs::createTemporaryFile("foo", "bar", FD, Path));
+  FileRemover Cleanup(Path);
+
+  ASSERT_THAT_ERROR(writeToOutput(Path,
+                                  [](raw_ostream &Out) -> Error {
+                                    Out << "HelloWorld";
+                                    return Error::success();
+                                  }),
+                    Succeeded());
+  checkFileData(Path, "HelloWorld");
+}
+
+TEST(raw_ostreamTest, writeToNonexistingPath) {
+  StringRef FileName = "/_bad/_path";
+  std::string ErrorMessage = toString(createFileError(
+      FileName, make_error_code(errc::no_such_file_or_directory)));
+
+  EXPECT_THAT_ERROR(writeToOutput(FileName,
+                                  [](raw_ostream &Out) -> Error {
+                                    Out << "HelloWorld";
+                                    return Error::success();
+                                  }),
+                    FailedWithMessage(ErrorMessage));
+}
+
+TEST(raw_ostreamTest, writeToDevNull) {
+  bool DevNullIsUsed = false;
+
+  EXPECT_THAT_ERROR(
+      writeToOutput("/dev/null",
+                    [&](raw_ostream &Out) -> Error {
+                      DevNullIsUsed =
+                          testing::internal::CheckedDowncastToActualType<
+                              raw_null_ostream, raw_ostream>(&Out);
+                      return Error::success();
+                    }),
+      Succeeded());
+
+  EXPECT_TRUE(DevNullIsUsed);
+}
+
+TEST(raw_ostreamTest, writeToStdOut) {
+  outs().flush();
+  testing::internal::CaptureStdout();
+
+  EXPECT_THAT_ERROR(writeToOutput("-",
+                                  [](raw_ostream &Out) -> Error {
+                                    Out << "HelloWorld";
+                                    return Error::success();
+                                  }),
+                    Succeeded());
+  outs().flush();
+
+  std::string CapturedStdOut = testing::internal::GetCapturedStdout();
+  EXPECT_EQ(CapturedStdOut, "HelloWorld");
+}
 }