[DWARFDataExtractor] Add a "truncating" constructor

[labath]

Summary

This constructor allows us to create a new DWARFDataExtractor which will
only present a subrange of an entire debug section. Since debug sections
typically consist of multiple contributions, it is expected that one
will create a new data extractor for each contribution in order to
avoid unexpectedly running off into the next one.

This is very useful for unifying the flows for detecting parse errors.
Without it, the code needs to consider two very different scenarios:

1. If there is another contribution after the current one, the DataExtractor functions will just start reading from there. This is detectable by comparing the current offset against the known end-of-contribution offset.
2. If this is the last contribution, the data extractor will just start returning zeroes (or other default values). This situation can *not* be detected by checking the parsing offset, as this will not be advanced in case of errors.

Using a truncated data extractor simplifies the code (and reduces
cognitive load) by making these two cases behave identically -- a
running off the end of a contribution will _always_ produce an EOF error
(if one uses error-aware parsing methods) or return default values.

[dblaikie]

Looks good - thanks! (I guess at some point we might want a DWARFDataExtractor that can be offset as well - for reading sections in DWP files (essentially create a view of the file based on the index, restricting a CU to only being able to see (& resolve references relative to) the regions described by the index))

[jhenderson]

llvm/include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h

42–46

I wonder if this should be more generic than the DWARFDataExtractor class, i.e. be in DataExtractor? The concept of limiting length to a specified amount is hardly DWARF specific - ELF sections, for example, sometimes need parsing and have a limited length.

Preumably of course, we'd still want this constructor, but it would just forward to the base class one.

llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp

197

I think it would make sense to show the behaviour where an entry can be partially read, i.e. the length truncates the field. I think we'd need this tested both for a location with a relocation and for one without.

[labath]

(Sorry about the delay, last week has been pretty busy for me.)

Replying to these two comments together:

In D77556#1964692, @dblaikie wrote:

Looks good - thanks! (I guess at some point we might want a DWARFDataExtractor that can be offset as well - for reading sections in DWP files (essentially create a view of the file based on the index, restricting a CU to only being able to see (& resolve references relative to) the regions described by the index))

@jhenderson wrote:

I wonder if this should be more generic than the DWARFDataExtractor class, i.e. be in DataExtractor? The concept of limiting length to a specified amount is hardly DWARF specific - ELF sections, for example, sometimes need parsing and have a limited length.

Preumably of course, we'd still want this constructor, but it would just forward to the base class one.

An offseting constructor for a DWARFDataExtractor would be slightly tricky because we would still need to use the "original" offset when resolving relocations. Nothing unsolvable, but it did not seem worth implementing when there's no use case -- dwp files don't have relocations and they deal with this by substr-ing the StringRef before they create a DWARFDataExtractor. The same approach can be used for regular DataExtractors, so a substr constructor is not strictly necessary there. However, it would be trivial to add, and it seems like it could come in handy sometimes, so I can add one if you want.

[labath]

• add more tests for reads which cross extractor boundaries. This required a fix to getRelocatedValue, which was spun out to a separate patch
• this patch does not (yet) introduce any additional constructors

[labath]

• add const & to Twine

[jhenderson]

LGTM, thanks. I don't know that we need the other constructor, but if there's other code that could be simplified by adding it, it might make sense (but probably could be a separate change).

llvm/include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h

public:
   DWARFDataExtractor(StringRef Data, bool IsLittleEndian, uint8_t AddressSize)
     : DataExtractor(Data, IsLittleEndian, AddressSize) {}
   DWARFDataExtractor(ArrayRef<uint8_t> Data, bool IsLittleEndian,
                      uint8_t AddressSize)
       : DataExtractor(
             StringRef(reinterpret_cast<const char *>(Data.data()), Data.size()),
             IsLittleEndian, AddressSize) {}
 
+  /// Truncating constructor
+  DWARFDataExtractor(const DWARFDataExtractor &Other, size_t Length)
+      : DataExtractor(Other.getData().substr(0, Length), Other.isLittleEndian(),
+                      Other.getAddressSize()),
+        Obj(Other.Obj), Section(Other.Section) {}
+
   /// Extracts the DWARF "initial length" field, which can either be a 32-bit
   /// value smaller than 0xfffffff0, or the value 0xffffffff followed by a
   /// 64-bit length. Returns the actual length, and the DWARF format which is
   /// encoded in the field. In case of errors, it returns {0, DWARF32} and
   /// leaves the offset unchanged.
   std::pair<uint64_t, dwarf::DwarfFormat>
   getInitialLength(uint64_t *Off, Error *Err = nullptr) const;
 

llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp

EXPECT_THAT_EXPECTED(
       GetWithError({0xff, 0xff, 0xff, 0xff, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05,
                     0x06, 0x07}),
       HasValue(std::make_tuple(0x0001020304050607, dwarf::DWARF64, 12)));
   EXPECT_EQ(GetWithoutError({0xff, 0xff, 0xff, 0xff, 0x00, 0x01, 0x02, 0x03,
                              0x04, 0x05, 0x06, 0x07}),
             std::make_tuple(0x0001020304050607, dwarf::DWARF64, 12));
 }
 
+TEST(DWARFDataExtractorTest, Truncation) {
+  StringRef Yaml = R"(
+!ELF
+FileHeader:
+  Class:    ELFCLASS32
+  Data:     ELFDATA2LSB
+  Type:     ET_REL
+  Machine:  EM_386
+Sections:
+  - Name:     .text
+    Type:     SHT_PROGBITS
+    Size:     0x80
+  - Name:     .debug_line
+    Type:     SHT_PROGBITS
+    Content:  '616263640000000065666768'
+  - Name:     .rel.debug_line
+    Type:     SHT_REL
+    Info:     .debug_line
+    Relocations:
+      - Offset:   4
+        Symbol:   f
+        Type:     R_386_32
+Symbols:
+  - Name:     f
+    Type:     STT_SECTION
+    Section:  .text
+    Value:    0x42
+)";
+  SmallString<0> Storage;
+  std::unique_ptr<object::ObjectFile> Obj = yaml::yaml2ObjectFile(
+      Storage, Yaml, [](const Twine &Err) { errs() << Err; });
+  ASSERT_TRUE(Obj);
+  std::unique_ptr<DWARFContext> Ctx = DWARFContext::create(*Obj);
+  const DWARFObject &DObj = Ctx->getDWARFObj();
+  ASSERT_EQ(12u, DObj.getLineSection().Data.size());
+
+  DWARFDataExtractor Data(DObj, DObj.getLineSection(), Obj->isLittleEndian(),
+                          Obj->getBytesInAddress());
+  DataExtractor::Cursor C(0);
+  EXPECT_EQ(0x64636261u, Data.getRelocatedAddress(C));
+  EXPECT_EQ(0x42u, Data.getRelocatedAddress(C));
+  EXPECT_EQ(0x68676665u, Data.getRelocatedAddress(C));
+  EXPECT_THAT_ERROR(C.takeError(), Succeeded());
+
+  C = DataExtractor::Cursor{0};
+  DWARFDataExtractor Truncated8(Data, 8);
+  EXPECT_EQ(0x64636261u, Truncated8.getRelocatedAddress(C));
+  EXPECT_EQ(0x42u, Truncated8.getRelocatedAddress(C));
+  EXPECT_EQ(0x0u, Truncated8.getRelocatedAddress(C));
+  EXPECT_THAT_ERROR(C.takeError(),
+                    FailedWithMessage("unexpected end of data at offset 0x8"));
+
+  C = DataExtractor::Cursor{0};
+  DWARFDataExtractor Truncated6(Data, 6);
+  EXPECT_EQ(0x64636261u, Truncated6.getRelocatedAddress(C));
+  EXPECT_EQ(0x0u, Truncated6.getRelocatedAddress(C));
+  EXPECT_THAT_ERROR(C.takeError(),
+                    FailedWithMessage("unexpected end of data at offset 0x4"));
+
+  C = DataExtractor::Cursor{0};
+  DWARFDataExtractor Truncated2(Data, 2);
+  EXPECT_EQ(0x0u, Truncated2.getRelocatedAddress(C));
+  EXPECT_THAT_ERROR(C.takeError(),
+                    FailedWithMessage("unexpected end of data at offset 0x0"));
+}
+
 } // namespace