MinidumpYAML: move serialization code to MinidumpEmitter.cpp

[labath]

Summary

The code for serializing minidumps was living in MinidumpYAML.cpp
so that it would be accessible from unit tests. While this had its
advantages, it was also unfortunate because it broke symmetry with all
other yaml2obj serializers.

Fortunately, nowadays all of yaml2obj is a library, so we don't need to
do anything special. This patch improves the code consistency by moving
the serialization code to MinidumpEmitter.cpp to match the style used in
other backends. It also removes the writeAsBinary entry point in favor
of the more general convertYAML interface.

This patch is just massaging the code a bit. There shouldn't be any
functional change here.

[jhenderson]

LGTM!

llvm/include/llvm/ObjectYAML/MinidumpYAML.h

struct Object {
   minidump::Header Header;
 
   /// The list of streams in this minidump object.
   std::vector<std::unique_ptr<Stream>> Streams;
 
   static Expected<Object> create(const object::MinidumpFile &File);
 };
 
-/// Serialize the minidump file represented by Obj to OS in binary form.
-void writeAsBinary(Object &Obj, raw_ostream &OS);
-
-/// Serialize the yaml string as a minidump file to OS in binary form.
-Error writeAsBinary(StringRef Yaml, raw_ostream &OS);
-
 } // namespace MinidumpYAML
 
 namespace yaml {
 template <> struct BlockScalarTraits<MinidumpYAML::BlockStringRef> {
   static void output(const MinidumpYAML::BlockStringRef &Text, void *,
                      raw_ostream &OS) {
     OS << Text;
   }

llvm/lib/ObjectYAML/MinidumpEmitter.cpp

 //===- yaml2minidump.cpp - Convert a YAML file to a minidump file ---------===//
 //
 // 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/ObjectYAML/MinidumpYAML.h"
 #include "llvm/ObjectYAML/yaml2obj.h"
+#include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
+using namespace llvm::minidump;
+using namespace llvm::MinidumpYAML;
+
+namespace {
+/// A helper class to manage the placement of various structures into the final
+/// minidump binary. Space for objects can be allocated via various allocate***
+/// methods, while the final minidump file is written by calling the writeTo
+/// method. The plain versions of allocation functions take a reference to the
+/// data which is to be written (and hence the data must be available until
+/// writeTo is called), while the "New" versions allocate the data in an
+/// allocator-managed buffer, which is available until the allocator object is
+/// destroyed. For both kinds of functions, it is possible to modify the
+/// data for which the space has been "allocated" until the final writeTo call.
+/// This is useful for "linking" the allocated structures via their offsets.
+class BlobAllocator {
+public:
+  size_t tell() const { return NextOffset; }
+
+  size_t allocateCallback(size_t Size,
+                          std::function<void(raw_ostream &)> Callback) {
+    size_t Offset = NextOffset;
+    NextOffset += Size;
+    Callbacks.push_back(std::move(Callback));
+    return Offset;
+  }
+
+  size_t allocateBytes(ArrayRef<uint8_t> Data) {
+    return allocateCallback(
+        Data.size(), [Data](raw_ostream &OS) { OS << toStringRef(Data); });
+  }
+
+  size_t allocateBytes(yaml::BinaryRef Data) {
+    return allocateCallback(Data.binary_size(), [Data](raw_ostream &OS) {
+      Data.writeAsBinary(OS);
+    });
+  }
+
+  template <typename T> size_t allocateArray(ArrayRef<T> Data) {
+    return allocateBytes({reinterpret_cast<const uint8_t *>(Data.data()),
+                          sizeof(T) * Data.size()});
+  }
+
+  template <typename T, typename RangeType>
+  std::pair<size_t, MutableArrayRef<T>>
+  allocateNewArray(const iterator_range<RangeType> &Range);
+
+  template <typename T> size_t allocateObject(const T &Data) {
+    return allocateArray(makeArrayRef(Data));
+  }
+
+  template <typename T, typename... Types>
+  std::pair<size_t, T *> allocateNewObject(Types &&... Args) {
+    T *Object = new (Temporaries.Allocate<T>()) T(std::forward<Types>(Args)...);
+    return {allocateObject(*Object), Object};
+  }
+
+  size_t allocateString(StringRef Str);
+
+  void writeTo(raw_ostream &OS) const;
+
+private:
+  size_t NextOffset = 0;
+
+  BumpPtrAllocator Temporaries;
+  std::vector<std::function<void(raw_ostream &)>> Callbacks;
+};
+} // namespace
+
+template <typename T, typename RangeType>
+std::pair<size_t, MutableArrayRef<T>>
+BlobAllocator::allocateNewArray(const iterator_range<RangeType> &Range) {
+  size_t Num = std::distance(Range.begin(), Range.end());
+  MutableArrayRef<T> Array(Temporaries.Allocate<T>(Num), Num);
+  std::uninitialized_copy(Range.begin(), Range.end(), Array.begin());
+  return {allocateArray(Array), Array};
+}
+
+size_t BlobAllocator::allocateString(StringRef Str) {
+  SmallVector<UTF16, 32> WStr;
+  bool OK = convertUTF8ToUTF16String(Str, WStr);
+  assert(OK && "Invalid UTF8 in Str?");
+  (void)OK;
+
+  // The utf16 string is null-terminated, but the terminator is not counted in
+  // the string size.
+  WStr.push_back(0);
+  size_t Result =
+      allocateNewObject<support::ulittle32_t>(2 * (WStr.size() - 1)).first;
+  allocateNewArray<support::ulittle16_t>(make_range(WStr.begin(), WStr.end()));
+  return Result;
+}
+
+void BlobAllocator::writeTo(raw_ostream &OS) const {
+  size_t BeginOffset = OS.tell();
+  for (const auto &Callback : Callbacks)
+    Callback(OS);
+  assert(OS.tell() == BeginOffset + NextOffset &&
+         "Callbacks wrote an unexpected number of bytes.");
+  (void)BeginOffset;
+}
+
+static LocationDescriptor layout(BlobAllocator &File, yaml::BinaryRef Data) {
+  return {support::ulittle32_t(Data.binary_size()),
+          support::ulittle32_t(File.allocateBytes(Data))};
+}
+
+static void layout(BlobAllocator &File, MemoryListStream::entry_type &Range) {
+  Range.Entry.Memory = layout(File, Range.Content);
+}
+
+static void layout(BlobAllocator &File, ModuleListStream::entry_type &M) {
+  M.Entry.ModuleNameRVA = File.allocateString(M.Name);
+
+  M.Entry.CvRecord = layout(File, M.CvRecord);
+  M.Entry.MiscRecord = layout(File, M.MiscRecord);
+}
+
+static void layout(BlobAllocator &File, ThreadListStream::entry_type &T) {
+  T.Entry.Stack.Memory = layout(File, T.Stack);
+  T.Entry.Context = layout(File, T.Context);
+}
+
+template <typename EntryT>
+static size_t layout(BlobAllocator &File,
+                     MinidumpYAML::detail::ListStream<EntryT> &S) {
+
+  File.allocateNewObject<support::ulittle32_t>(S.Entries.size());
+  for (auto &E : S.Entries)
+    File.allocateObject(E.Entry);
+
+  size_t DataEnd = File.tell();
+
+  // Lay out the auxiliary data, (which is not a part of the stream).
+  DataEnd = File.tell();
+  for (auto &E : S.Entries)
+    layout(File, E);
+
+  return DataEnd;
+}
+
+static Directory layout(BlobAllocator &File, Stream &S) {
+  Directory Result;
+  Result.Type = S.Type;
+  Result.Location.RVA = File.tell();
+  Optional<size_t> DataEnd;
+  switch (S.Kind) {
+  case Stream::StreamKind::MemoryList:
+    DataEnd = layout(File, cast<MemoryListStream>(S));
+    break;
+  case Stream::StreamKind::ModuleList:
+    DataEnd = layout(File, cast<ModuleListStream>(S));
+    break;
+  case Stream::StreamKind::RawContent: {
+    RawContentStream &Raw = cast<RawContentStream>(S);
+    File.allocateCallback(Raw.Size, [&Raw](raw_ostream &OS) {
+      Raw.Content.writeAsBinary(OS);
+      assert(Raw.Content.binary_size() <= Raw.Size);
+      OS << std::string(Raw.Size - Raw.Content.binary_size(), '\0');
+    });
+    break;
+  }
+  case Stream::StreamKind::SystemInfo: {
+    SystemInfoStream &SystemInfo = cast<SystemInfoStream>(S);
+    File.allocateObject(SystemInfo.Info);
+    // The CSD string is not a part of the stream.
+    DataEnd = File.tell();
+    SystemInfo.Info.CSDVersionRVA = File.allocateString(SystemInfo.CSDVersion);
+    break;
+  }
+  case Stream::StreamKind::TextContent:
+    File.allocateArray(arrayRefFromStringRef(cast<TextContentStream>(S).Text));
+    break;
+  case Stream::StreamKind::ThreadList:
+    DataEnd = layout(File, cast<ThreadListStream>(S));
+    break;
+  }
+  // If DataEnd is not set, we assume everything we generated is a part of the
+  // stream.
+  Result.Location.DataSize =
+      DataEnd.getValueOr(File.tell()) - Result.Location.RVA;
+  return Result;
+}
 
 namespace llvm {
 namespace yaml {
 
-int yaml2minidump(MinidumpYAML::Object &Doc, raw_ostream &Out) {
-  writeAsBinary(Doc, Out);
+int yaml2minidump(MinidumpYAML::Object &Obj, raw_ostream &Out) {
+  BlobAllocator File;
+  File.allocateObject(Obj.Header);
+
+  std::vector<Directory> StreamDirectory(Obj.Streams.size());
+  Obj.Header.StreamDirectoryRVA =
+      File.allocateArray(makeArrayRef(StreamDirectory));
+  Obj.Header.NumberOfStreams = StreamDirectory.size();
+
+  for (auto &Stream : enumerate(Obj.Streams))
+    StreamDirectory[Stream.index()] = layout(File, *Stream.value());
+
+  File.writeTo(Out);
   return 0;
 }
 
 } // namespace yaml
 } // namespace llvm

llvm/lib/ObjectYAML/MinidumpYAML.cpp

 //===- MinidumpYAML.cpp - Minidump YAMLIO implementation ------------------===//
 //
 // 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/ObjectYAML/MinidumpYAML.h"
 #include "llvm/Support/Allocator.h"
-#include "llvm/Support/ConvertUTF.h"
 
 using namespace llvm;
 using namespace llvm::MinidumpYAML;
 using namespace llvm::minidump;
 
-namespace {
-/// A helper class to manage the placement of various structures into the final
-/// minidump binary. Space for objects can be allocated via various allocate***
-/// methods, while the final minidump file is written by calling the writeTo
-/// method. The plain versions of allocation functions take a reference to the
-/// data which is to be written (and hence the data must be available until
-/// writeTo is called), while the "New" versions allocate the data in an
-/// allocator-managed buffer, which is available until the allocator object is
-/// destroyed. For both kinds of functions, it is possible to modify the
-/// data for which the space has been "allocated" until the final writeTo call.
-/// This is useful for "linking" the allocated structures via their offsets.
-class BlobAllocator {
-public:
-  size_t tell() const { return NextOffset; }
-
-  size_t allocateCallback(size_t Size,
-                          std::function<void(raw_ostream &)> Callback) {
-    size_t Offset = NextOffset;
-    NextOffset += Size;
-    Callbacks.push_back(std::move(Callback));
-    return Offset;
-  }
-
-  size_t allocateBytes(ArrayRef<uint8_t> Data) {
-    return allocateCallback(
-        Data.size(), [Data](raw_ostream &OS) { OS << toStringRef(Data); });
-  }
-
-  size_t allocateBytes(yaml::BinaryRef Data) {
-    return allocateCallback(Data.binary_size(), [Data](raw_ostream &OS) {
-      Data.writeAsBinary(OS);
-    });
-  }
-
-  template <typename T> size_t allocateArray(ArrayRef<T> Data) {
-    return allocateBytes({reinterpret_cast<const uint8_t *>(Data.data()),
-                          sizeof(T) * Data.size()});
-  }
-
-  template <typename T, typename RangeType>
-  std::pair<size_t, MutableArrayRef<T>>
-  allocateNewArray(const iterator_range<RangeType> &Range);
-
-  template <typename T> size_t allocateObject(const T &Data) {
-    return allocateArray(makeArrayRef(Data));
-  }
-
-  template <typename T, typename... Types>
-  std::pair<size_t, T *> allocateNewObject(Types &&... Args) {
-    T *Object = new (Temporaries.Allocate<T>()) T(std::forward<Types>(Args)...);
-    return {allocateObject(*Object), Object};
-  }
-
-  size_t allocateString(StringRef Str);
-
-  void writeTo(raw_ostream &OS) const;
-
-private:
-  size_t NextOffset = 0;
-
-  BumpPtrAllocator Temporaries;
-  std::vector<std::function<void(raw_ostream &)>> Callbacks;
-};
-} // namespace
-
-template <typename T, typename RangeType>
-std::pair<size_t, MutableArrayRef<T>>
-BlobAllocator::allocateNewArray(const iterator_range<RangeType> &Range) {
-  size_t Num = std::distance(Range.begin(), Range.end());
-  MutableArrayRef<T> Array(Temporaries.Allocate<T>(Num), Num);
-  std::uninitialized_copy(Range.begin(), Range.end(), Array.begin());
-  return {allocateArray(Array), Array};
-}
-
-size_t BlobAllocator::allocateString(StringRef Str) {
-  SmallVector<UTF16, 32> WStr;
-  bool OK = convertUTF8ToUTF16String(Str, WStr);
-  assert(OK && "Invalid UTF8 in Str?");
-  (void)OK;
-
-  // The utf16 string is null-terminated, but the terminator is not counted in
-  // the string size.
-  WStr.push_back(0);
-  size_t Result =
-      allocateNewObject<support::ulittle32_t>(2 * (WStr.size() - 1)).first;
-  allocateNewArray<support::ulittle16_t>(make_range(WStr.begin(), WStr.end()));
-  return Result;
-}
-
-void BlobAllocator::writeTo(raw_ostream &OS) const {
-  size_t BeginOffset = OS.tell();
-  for (const auto &Callback : Callbacks)
-    Callback(OS);
-  assert(OS.tell() == BeginOffset + NextOffset &&
-         "Callbacks wrote an unexpected number of bytes.");
-  (void)BeginOffset;
-}
-
 /// Perform an optional yaml-mapping of an endian-aware type EndianType. The
 /// only purpose of this function is to avoid casting the Default value to the
 /// endian type;
 template <typename EndianType>
 static inline void mapOptional(yaml::IO &IO, const char *Key, EndianType &Val,
                                typename EndianType::value_type Default) {
   IO.mapOptional(Key, Val, EndianType(Default));
 }

 void yaml::MappingTraits<Object>::mapping(IO &IO, Object &O) {
   IO.mapTag("!minidump", true);
   mapOptionalHex(IO, "Signature", O.Header.Signature, Header::MagicSignature);
   mapOptionalHex(IO, "Version", O.Header.Version, Header::MagicVersion);
   mapOptionalHex(IO, "Flags", O.Header.Flags, 0);
   IO.mapRequired("Streams", O.Streams);
 }
 
-static LocationDescriptor layout(BlobAllocator &File, yaml::BinaryRef Data) {
-  return {support::ulittle32_t(Data.binary_size()),
-          support::ulittle32_t(File.allocateBytes(Data))};
-}
-
-static void layout(BlobAllocator &File, MemoryListStream::entry_type &Range) {
-  Range.Entry.Memory = layout(File, Range.Content);
-}
-
-static void layout(BlobAllocator &File, ModuleListStream::entry_type &M) {
-  M.Entry.ModuleNameRVA = File.allocateString(M.Name);
-
-  M.Entry.CvRecord = layout(File, M.CvRecord);
-  M.Entry.MiscRecord = layout(File, M.MiscRecord);
-}
-
-static void layout(BlobAllocator &File, ThreadListStream::entry_type &T) {
-  T.Entry.Stack.Memory = layout(File, T.Stack);
-  T.Entry.Context = layout(File, T.Context);
-}
-
-template <typename EntryT>
-static size_t layout(BlobAllocator &File,
-                     MinidumpYAML::detail::ListStream<EntryT> &S) {
-
-  File.allocateNewObject<support::ulittle32_t>(S.Entries.size());
-  for (auto &E : S.Entries)
-    File.allocateObject(E.Entry);
-
-  size_t DataEnd = File.tell();
-
-  // Lay out the auxiliary data, (which is not a part of the stream).
-  DataEnd = File.tell();
-  for (auto &E : S.Entries)
-    layout(File, E);
-
-  return DataEnd;
-}
-
-static Directory layout(BlobAllocator &File, Stream &S) {
-  Directory Result;
-  Result.Type = S.Type;
-  Result.Location.RVA = File.tell();
-  Optional<size_t> DataEnd;
-  switch (S.Kind) {
-  case Stream::StreamKind::MemoryList:
-    DataEnd = layout(File, cast<MemoryListStream>(S));
-    break;
-  case Stream::StreamKind::ModuleList:
-    DataEnd = layout(File, cast<ModuleListStream>(S));
-    break;
-  case Stream::StreamKind::RawContent: {
-    RawContentStream &Raw = cast<RawContentStream>(S);
-    File.allocateCallback(Raw.Size, [&Raw](raw_ostream &OS) {
-      Raw.Content.writeAsBinary(OS);
-      assert(Raw.Content.binary_size() <= Raw.Size);
-      OS << std::string(Raw.Size - Raw.Content.binary_size(), '\0');
-    });
-    break;
-  }
-  case Stream::StreamKind::SystemInfo: {
-    SystemInfoStream &SystemInfo = cast<SystemInfoStream>(S);
-    File.allocateObject(SystemInfo.Info);
-    // The CSD string is not a part of the stream.
-    DataEnd = File.tell();
-    SystemInfo.Info.CSDVersionRVA = File.allocateString(SystemInfo.CSDVersion);
-    break;
-  }
-  case Stream::StreamKind::TextContent:
-    File.allocateArray(arrayRefFromStringRef(cast<TextContentStream>(S).Text));
-    break;
-  case Stream::StreamKind::ThreadList:
-    DataEnd = layout(File, cast<ThreadListStream>(S));
-    break;
-  }
-  // If DataEnd is not set, we assume everything we generated is a part of the
-  // stream.
-  Result.Location.DataSize =
-      DataEnd.getValueOr(File.tell()) - Result.Location.RVA;
-  return Result;
-}
-
-void MinidumpYAML::writeAsBinary(Object &Obj, raw_ostream &OS) {
-  BlobAllocator File;
-  File.allocateObject(Obj.Header);
-
-  std::vector<Directory> StreamDirectory(Obj.Streams.size());
-  Obj.Header.StreamDirectoryRVA =
-      File.allocateArray(makeArrayRef(StreamDirectory));
-  Obj.Header.NumberOfStreams = StreamDirectory.size();
-
-  for (auto &Stream : enumerate(Obj.Streams))
-    StreamDirectory[Stream.index()] = layout(File, *Stream.value());
-
-  File.writeTo(OS);
-}
-
-Error MinidumpYAML::writeAsBinary(StringRef Yaml, raw_ostream &OS) {
-  yaml::Input Input(Yaml);
-  Object Obj;
-  Input >> Obj;
-  if (std::error_code EC = Input.error())
-    return errorCodeToError(EC);
-
-  writeAsBinary(Obj, OS);
-  return Error::success();
-}
-
 Expected<std::unique_ptr<Stream>>
 Stream::create(const Directory &StreamDesc, const object::MinidumpFile &File) {
   StreamKind Kind = getKind(StreamDesc.Type);
   switch (Kind) {
   case StreamKind::MemoryList: {
     auto ExpectedList = File.getMemoryList();
     if (!ExpectedList)
       return ExpectedList.takeError();

llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp

 //===- MinidumpYAMLTest.cpp - Tests for Minidump<->YAML code --------------===//
 //
 // 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/ObjectYAML/MinidumpYAML.h"
 #include "llvm/Object/Minidump.h"
-#include "llvm/ObjectYAML/ObjectYAML.h"
+#include "llvm/ObjectYAML/yaml2obj.h"
+#include "llvm/Support/YAMLTraits.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 using namespace llvm::minidump;
 
 static Expected<std::unique_ptr<object::MinidumpFile>>
 toBinary(SmallVectorImpl<char> &Storage, StringRef Yaml) {
   Storage.clear();
   raw_svector_ostream OS(Storage);
-  if (Error E = MinidumpYAML::writeAsBinary(Yaml, OS))
+  yaml::Input YIn(Yaml);
+  if (Error E = yaml::convertYAML(YIn, OS))
     return std::move(E);
 
   return object::MinidumpFile::create(MemoryBufferRef(OS.str(), "Binary"));
 }
 
 TEST(MinidumpYAML, Basic) {
   SmallString<0> Storage;
   auto ExpectedFile = toBinary(Storage, R"(