Drop RVA property and index and instead pop off the front of the arrayrefs, remove expected method and use range loop

Author: Jlalond

llvm/include/llvm/Object/Minidump.h

@@ -146,15 +146,12 @@ class MinidumpFile : public Binary {
   public:
     static Memory64Iterator
     begin(ArrayRef<uint8_t> Storage,
-          ArrayRef<minidump::MemoryDescriptor_64> Descriptors,
-          uint64_t BaseRVA) {
-      return Memory64Iterator(Storage, Descriptors, BaseRVA);
+          ArrayRef<minidump::MemoryDescriptor_64> Descriptors) {
+      return Memory64Iterator(Storage, Descriptors);
     }
 
     static Memory64Iterator end() { return Memory64Iterator(); }
 
-    std::pair<minidump::MemoryDescriptor_64, ArrayRef<uint8_t>> Current;
-
     bool operator==(const Memory64Iterator &R) const {
       return IsEnd == R.IsEnd;
     }
@@ -167,29 +164,33 @@ class MinidumpFile : public Binary {
     }
 
     const std::pair<minidump::MemoryDescriptor_64, ArrayRef<uint8_t>> *
-    operator&() {
+    operator->() {
       return &Current;
     }
 
     Error inc() {
-      if (Storage.size() == 0 || Descriptors.size() == 0)
-        return make_error<GenericBinaryError>("No Memory64List Stream",
-                                              object_error::parse_failed);
-
-      if (Index >= Descriptors.size())
-        return createError("Can't read past of Memory64List Stream");
+      if (Storage.size() == 0 || Descriptors.size() == 0) {
+        IsEnd = true;
+        return Error::success();
+      }
 
-      const minidump::MemoryDescriptor_64 &Descriptor = Descriptors[Index];
-      if (RVA + Descriptor.DataSize > Storage.size())
+      // Drop front gives us an array ref, so we need to call .front() as well.
+      const minidump::MemoryDescriptor_64 &Descriptor = Descriptors.front();
+      if (Descriptor.DataSize > Storage.size()) {
+        IsEnd = true;
         return make_error<GenericBinaryError>(
             "Memory64 Descriptor exceeds end of file.",
             object_error::unexpected_eof);
+      }
 
-      ArrayRef<uint8_t> Content = Storage.slice(RVA, Descriptor.DataSize);
+
+      ArrayRef<uint8_t> Content = Storage.take_front(Descriptor.DataSize);
       Current = std::make_pair(Descriptor, Content);
-      Index++;
-      RVA += Descriptor.DataSize;
-      if (Index >= Descriptors.size())
+
+      Storage = Storage.drop_front(Descriptor.DataSize);
+      Descriptors = Descriptors.drop_front();
+
+      if (Descriptors.empty())
         IsEnd = true;
       return Error::success();
     }
@@ -198,23 +199,22 @@ class MinidumpFile : public Binary {
     // This constructor will only happen after a validation check to see
     // if the first descriptor is readable.
     Memory64Iterator(ArrayRef<uint8_t> Storage,
-                     ArrayRef<minidump::MemoryDescriptor_64> Descriptors,
-                     uint64_t BaseRVA)
-        : RVA(BaseRVA), Storage(Storage), Descriptors(Descriptors),
+                     ArrayRef<minidump::MemoryDescriptor_64> Descriptors)
+        : Storage(Storage), Descriptors(Descriptors),
           IsEnd(false) {
-      assert(Descriptors.size() > 0);
-      assert(Storage.size() >= BaseRVA + Descriptors.front().DataSize);
-      Current =
-          std::make_pair(Descriptors.front(),
-                         Storage.slice(BaseRVA, Descriptors.front().DataSize));
+      assert(Descriptors.size() > 0 && Storage.size() >= Descriptors.front().DataSize);
+      minidump::MemoryDescriptor_64 Descriptor = Descriptors.front();
+      ArrayRef<uint8_t> Content = Storage.take_front(Descriptor.DataSize);
+      Current = std::make_pair(Descriptor, Content);
+      this->Descriptors = Descriptors.drop_front();
+      this->Storage = Storage.drop_front(Descriptor.DataSize);
     }
 
     Memory64Iterator()
-        : RVA(0), Storage(ArrayRef<uint8_t>()),
+        : Storage(ArrayRef<uint8_t>()),
           Descriptors(ArrayRef<minidump::MemoryDescriptor_64>()), IsEnd(true) {}
 
-    size_t Index = 0;
-    uint64_t RVA;
+    std::pair<minidump::MemoryDescriptor_64, ArrayRef<uint8_t>> Current;
     ArrayRef<uint8_t> Storage;
     ArrayRef<minidump::MemoryDescriptor_64> Descriptors;
     bool IsEnd;
@@ -226,7 +226,7 @@ class MinidumpFile : public Binary {
   /// content from the Memory64List stream. An error is returned if the file
   /// does not contain a Memory64List stream, or if the descriptor data is
   /// unreadable.
-  Expected<iterator_range<FallibleMemory64Iterator>>
+  iterator_range<FallibleMemory64Iterator>
   getMemory64List(Error &Err) const;
 
   /// Returns the list of descriptors embedded in the MemoryInfoList stream. The

llvm/lib/Object/Minidump.cpp

@@ -156,32 +156,45 @@ MinidumpFile::create(MemoryBufferRef Source) {
       new MinidumpFile(Source, Hdr, *ExpectedStreams, std::move(StreamMap)));
 }
 
-Expected<iterator_range<MinidumpFile::FallibleMemory64Iterator>>
+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 {
   Expected<minidump::Memory64ListHeader> ListHeader = getMemoryList64Header();
-  if (!ListHeader)
-    return ListHeader.takeError();
+  if (!ListHeader) {
+    Err = ListHeader.takeError();
+    return makeEmptyRange(Err);
+  }
 
   std::optional<ArrayRef<uint8_t>> Stream =
       getRawStream(StreamType::Memory64List);
-  if (!Stream)
-    return createError("No such stream");
+  if (!Stream) {
+    Err = createError("No such stream");
+    return makeEmptyRange(Err);
+  }
 
   Expected<ArrayRef<minidump::MemoryDescriptor_64>> Descriptors =
       getDataSliceAs<minidump::MemoryDescriptor_64>(
           *Stream, sizeof(Memory64ListHeader),
           ListHeader->NumberOfMemoryRanges);
 
-  if (!Descriptors)
-    return Descriptors.takeError();
+  if (!Descriptors) {
+    Err = Descriptors.takeError();
+    return makeEmptyRange(Err);
+  }
 
   if (!Descriptors->empty() &&
-      ListHeader->BaseRVA + Descriptors->front().DataSize > getData().size())
-    return createEOFError();
+      ListHeader->BaseRVA + Descriptors->front().DataSize > getData().size()) {
+    Err = createError("Memory64List header RVA out of range");
+    return makeEmptyRange(Err);
+  }
 
   return make_range(
       FallibleMemory64Iterator::itr(
-          Memory64Iterator::begin(getData(), *Descriptors, ListHeader->BaseRVA),
+          Memory64Iterator::begin(getData().slice(ListHeader->BaseRVA), *Descriptors),
           Err),
       FallibleMemory64Iterator::end(Memory64Iterator::end()));
 }

llvm/lib/ObjectYAML/MinidumpYAML.cpp

@@ -529,22 +529,13 @@ Stream::create(const Directory &StreamDesc, const object::MinidumpFile &File) {
     return std::make_unique<MemoryListStream>(std::move(Ranges));
   }
   case StreamKind::Memory64List: {
-    // Error, unlike expected is true in failure state
     Error Err = Error::success();
-    // Explicit check on Err so that if we return due to getmemory64list
-    // getting an error, it's not destructed when unchecked.
+    auto Memory64List = File.getMemory64List(Err);
     if (Err)
       return Err;
-    auto ExpectedList = File.getMemory64List(Err);
-    if (!ExpectedList)
-      return ExpectedList.takeError();
     std::vector<Memory64ListStream::entry_type> Ranges;
-    for (auto It = ExpectedList->begin(); It != ExpectedList->end(); ++It) {
-      const auto Pair = *It;
-      if (!Err)
+    for (const auto &Pair : Memory64List) {
         Ranges.push_back({Pair.first, Pair.second});
-      else
-        break;
     }
 
     // If we don't have an error, or if any of the reads succeed, return ranges

llvm/unittests/ObjectYAML/MinidumpYAMLTest.cpp

@@ -353,7 +353,7 @@ TEST(MinidumpYAML, MemoryRegion_64bit) {
   ASSERT_THAT_EXPECTED(ExpectedFile, Succeeded());
   object::MinidumpFile &File = **ExpectedFile;
 
-  ASSERT_THAT(1u, File.streams().size());
+  ASSERT_THAT(File.streams().size(), 1u);
 
   Error Err = Error::success();
   // Explicit Err check
@@ -367,21 +367,22 @@ TEST(MinidumpYAML, MemoryRegion_64bit) {
       *ExpectedMemoryList;
   auto Iterator = MemoryList.begin();
 
-  ++Iterator;
-  ASSERT_FALSE(Err);
+
 
   auto DescOnePair = *Iterator;
   const minidump::MemoryDescriptor_64 &DescOne = DescOnePair.first;
-  ASSERT_THAT(0x7FFFFFCF0818283u, DescOne.StartOfMemoryRange);
-  ASSERT_THAT(5u, DescOne.DataSize);
-
+  ASSERT_THAT(DescOne.StartOfMemoryRange, 0x7FFFFFCF0818283u);
+  ASSERT_THAT(DescOne.DataSize, 5u);
   ++Iterator;
   ASSERT_FALSE(Err);
 
   auto DescTwoPair = *Iterator;
   const minidump::MemoryDescriptor_64 &DescTwo = DescTwoPair.first;
-  ASSERT_THAT(0x7FFFFFFF0818283u, DescTwo.StartOfMemoryRange);
-  ASSERT_THAT(5u, DescTwo.DataSize);
+  ASSERT_THAT(DescTwo.StartOfMemoryRange, 0x7FFFFFFF0818283u);
+  ASSERT_THAT(DescTwo.DataSize, 5u);
+  ++Iterator;
+  ASSERT_FALSE(Err);
+
   const std::optional<ArrayRef<uint8_t>> ExpectedContent =
       File.getRawStream(StreamType::Memory64List);
   ASSERT_TRUE(ExpectedContent);
@@ -396,14 +397,14 @@ TEST(MinidumpYAML, MemoryRegion_64bit) {
 
   Expected<ArrayRef<uint8_t>> DescOneExpectedContentSlice = DescOnePair.second;
   ASSERT_THAT_EXPECTED(DescOneExpectedContentSlice, Succeeded());
-  ASSERT_THAT(5u, DescOneExpectedContentSlice->size());
-  ASSERT_THAT(arrayRefFromStringRef("hello"), *DescOneExpectedContentSlice);
+  ASSERT_THAT(DescOneExpectedContentSlice->size(), 5u);
+  ASSERT_THAT(*DescOneExpectedContentSlice, arrayRefFromStringRef("hello"));
 
   Expected<ArrayRef<uint8_t>> DescTwoExpectedContentSlice = DescTwoPair.second;
   ASSERT_THAT_EXPECTED(DescTwoExpectedContentSlice, Succeeded());
-  ASSERT_THAT(arrayRefFromStringRef("world"), *DescTwoExpectedContentSlice);
+  ASSERT_THAT(*DescTwoExpectedContentSlice, arrayRefFromStringRef("world"));
 
-  ASSERT_TRUE(Iterator == MemoryList.end());
+  ASSERT_EQ(Iterator, MemoryList.end());
 }
 
 TEST(MinidumpYAML, MemoryRegion_DataSize_TooSmall) {
@@ -414,7 +415,7 @@ TEST(MinidumpYAML, MemoryRegion_DataSize_TooSmall) {
   - Type:            Memory64List
     Memory Ranges:
       - Start of Memory Range: 0x7FFFFFCF0818283
-        Data Size: 4           1
+        Data Size:             1
         Content:               '68656c6c6f'
       - Start of Memory Range: 0x7FFFFFFF0818283
         Content:               '776f726c64'