Impliment good feedback on making code more concise, and fix callback to pad 0's if datasize exceeds content

Author: Jlalond

llvm/include/llvm/Object/Minidump.h

@@ -189,8 +189,6 @@ class MinidumpFile : public Binary {
       Storage = Storage.drop_front(Descriptor.DataSize);
       Descriptors = Descriptors.drop_front();
 
-      if (Descriptors.empty())
-        IsEnd = true;
       return Error::success();
     }
 

llvm/lib/Object/Minidump.cpp

@@ -156,28 +156,21 @@ MinidumpFile::create(MemoryBufferRef Source) {
       new MinidumpFile(Source, Hdr, *ExpectedStreams, std::move(StreamMap)));
 }
 
-static iterator_range<MinidumpFile::FallibleMemory64Iterator>
-makeEmptyRange(Error &Err) {
-  return make_range(
-      llvm::object::MinidumpFile::FallibleMemory64Iterator::itr(
-          llvm::object::MinidumpFile::Memory64Iterator::end(), Err),
-      llvm::object::MinidumpFile::FallibleMemory64Iterator::end(
-          llvm::object::MinidumpFile::Memory64Iterator::end()));
-}
-
 iterator_range<MinidumpFile::FallibleMemory64Iterator>
 MinidumpFile::getMemory64List(Error &Err) const {
+  ErrorAsOutParameter ErrAsOutParam(&Err);
+  auto end = FallibleMemory64Iterator::end(Memory64Iterator::end());
   Expected<minidump::Memory64ListHeader> ListHeader = getMemoryList64Header();
   if (!ListHeader) {
     Err = ListHeader.takeError();
-    return makeEmptyRange(Err);
+    return make_range(end, end);
   }
 
   std::optional<ArrayRef<uint8_t>> Stream =
       getRawStream(StreamType::Memory64List);
   if (!Stream) {
     Err = createError("No such stream");
-    return makeEmptyRange(Err);
+    return make_range(end, end);
   }
 
   Expected<ArrayRef<minidump::MemoryDescriptor_64>> Descriptors =
@@ -187,13 +180,13 @@ MinidumpFile::getMemory64List(Error &Err) const {
 
   if (!Descriptors) {
     Err = Descriptors.takeError();
-    return makeEmptyRange(Err);
+    return make_range(end, end);
   }
 
   if (!Descriptors->empty() &&
       ListHeader->BaseRVA + Descriptors->front().DataSize > getData().size()) {
     Err = createError("Memory64List header RVA out of range");
-    return makeEmptyRange(Err);
+    return make_range(end, end);
   }
 
   return make_range(FallibleMemory64Iterator::itr(

llvm/lib/ObjectYAML/MinidumpEmitter.cpp

@@ -147,8 +147,15 @@ static size_t layout(BlobAllocator &File, MinidumpYAML::Memory64ListStream &S) {
 
   // Save the new offset for the stream size.
   size_t DataEnd = File.tell();
-  for (auto &E : S.Entries)
+  for (auto &E : S.Entries) {
     File.allocateBytes(E.Content);
+    if (E.Entry.DataSize > E.Content.binary_size()) {
+      size_t Padding = E.Entry.DataSize - E.Content.binary_size();
+      File.allocateCallback(Padding, [Padding](raw_ostream &OS) {
+        OS << std::string(Padding, '\0');
+      });
+    }
+  }
 
   return DataEnd;
 }

llvm/lib/ObjectYAML/MinidumpYAML.cpp

@@ -531,19 +531,14 @@ Stream::create(const Directory &StreamDesc, const object::MinidumpFile &File) {
   case StreamKind::Memory64List: {
     Error Err = Error::success();
     auto Memory64List = File.getMemory64List(Err);
-    if (Err)
-      return Err;
     std::vector<Memory64ListStream::entry_type> Ranges;
     for (const auto &Pair : Memory64List) {
       Ranges.push_back({Pair.first, Pair.second});
     }
 
-    // If we don't have an error, or if any of the reads succeed, return ranges
-    // this would also work if we have no descriptors.
-    if (!Err || Ranges.size() > 0)
-      return std::make_unique<Memory64ListStream>(std::move(Ranges));
-
-    return Err;
+    if (Err)
+      return Err;
+    return std::make_unique<Memory64ListStream>(std::move(Ranges));
   }
   case StreamKind::ModuleList: {
     auto ExpectedList = File.getModuleList();

llvm/test/tools/obj2yaml/Minidump/basic.yaml

@@ -74,6 +74,9 @@ Streams:
     Memory Ranges:
       - Start of Memory Range: 0x7FFFFFCF08180283
         Content:               '68656c6c6f776f726c64'
+      - Start of Memory Range: 0x7FFAFFCF08180283
+        Data Size:              8
+        Content:               '8008'
   - Type:            MemoryInfoList
     Memory Ranges:
       - Base Address:    0x0000000000000000
@@ -167,6 +170,8 @@ Streams:
 # CHECK-NEXT:    Memory Ranges:
 # CHECK-NEXT:      - Start of Memory Range: 0x7FFFFFCF08180283
 # CHECK-NEXT:        Content:         68656C6C6F776F726C64
+# CHECK-NEXT:      - Start of Memory Range: 0x7FFAFFCF08180283
+# CHECK-NEXT:        Content:         '8008000000000000'
 # CHECK-NEXT:   - Type:            MemoryInfoList
 # CHECK-NEXT:     Memory Ranges:
 # CHECK-NEXT:       - Base Address:       0x0

llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp

@@ -356,37 +356,32 @@ TEST(MinidumpYAML, MemoryRegion_64bit) {
   ASSERT_THAT(File.streams().size(), 1u);
 
   Error Err = Error::success();
-  // Explicit Err check
-  ASSERT_FALSE(Err);
-  Expected<iterator_range<object::MinidumpFile::FallibleMemory64Iterator>>
-      ExpectedMemoryList = File.getMemory64List(Err);
-
-  ASSERT_THAT_EXPECTED(ExpectedMemoryList, Succeeded());
-
   iterator_range<object::MinidumpFile::FallibleMemory64Iterator> MemoryList =
-      *ExpectedMemoryList;
+      File.getMemory64List(Err);
+
+  ASSERT_THAT_ERROR(std::move(Err), Succeeded());
   auto Iterator = MemoryList.begin();
 
   auto DescOnePair = *Iterator;
   const minidump::MemoryDescriptor_64 &DescOne = DescOnePair.first;
   ASSERT_THAT(DescOne.StartOfMemoryRange, 0x7FFFFFCF0818283u);
   ASSERT_THAT(DescOne.DataSize, 5u);
   ++Iterator;
-  ASSERT_FALSE(Err);
+  ASSERT_THAT_ERROR(std::move(Err), Succeeded());
 
   auto DescTwoPair = *Iterator;
   const minidump::MemoryDescriptor_64 &DescTwo = DescTwoPair.first;
   ASSERT_THAT(DescTwo.StartOfMemoryRange, 0x7FFFFFFF0818283u);
   ASSERT_THAT(DescTwo.DataSize, 5u);
   ++Iterator;
-  ASSERT_FALSE(Err);
+  ASSERT_THAT_ERROR(std::move(Err), Succeeded());
 
   const std::optional<ArrayRef<uint8_t>> ExpectedContent =
       File.getRawStream(StreamType::Memory64List);
   ASSERT_TRUE(ExpectedContent);
   const size_t ExpectedStreamSize =
       sizeof(Memory64ListHeader) + (sizeof(MemoryDescriptor_64) * 2);
-  ASSERT_THAT(ExpectedStreamSize, ExpectedContent->size());
+  ASSERT_THAT(ExpectedContent->size(), ExpectedStreamSize);
 
   Expected<minidump::Memory64ListHeader> ExpectedHeader =
       File.getMemoryList64Header();
@@ -404,20 +399,3 @@ TEST(MinidumpYAML, MemoryRegion_64bit) {
 
   ASSERT_EQ(Iterator, MemoryList.end());
 }
-
-TEST(MinidumpYAML, MemoryRegion_DataSize_TooSmall) {
-  SmallString<0> Storage;
-  auto ExpectedFile = toBinary(Storage, R"(
---- !minidump
-Streams:
-  - Type:            Memory64List
-    Memory Ranges:
-      - Start of Memory Range: 0x7FFFFFCF0818283
-        Data Size:             1
-        Content:               '68656c6c6f'
-      - Start of Memory Range: 0x7FFFFFFF0818283
-        Content:               '776f726c64'
-        )");
-
-  ASSERT_THAT_EXPECTED(ExpectedFile, Failed());
-}