Introduce DWARFDataExtractor::getInitialLength

[labath]

Summary

This patch introduces a function to house the code needed to do the
DWARF64 detection dance. It also updates three instances of DWARF64
parsing code to use this function instead of the hand-rolled
implementation.

This patch does _not_ attempt to handle the problem of detecting lengths
which extend past the size of the section, or cases when reads of a
single contribution accidentally escape beyond its specified length, but
I think it's useful in its own right. The idea is that this functionality will
be built on top of the function introduced here.

[ikudrin]

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

45

Would it be simpler if you implement getInitialLength(uint64_t *, Error *) via getInitialLength(Cursor &)? You anyway create a Cursor inside getInitialLength(uint64_t *, Error *).

[labath]

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

45

I don't think that would help very much (or at all). The second cursor comes from a desire to not modify the original offset in case we fail to read the entire initial length field. I am not sure if any of the potential users would really need this behavior, but that is how the rest of the DataExtractor functions operate, so I thought it'd be good to maintain that.

If we stick to that requirement, then I'd still need to have two cursors (or offsets, or something) and fiddle around with them. If we drop it, we wouldn't need the second Cursor even in the current setup.

[labath]

• add a unit test
• update debug_line parser to use the new method

This should be ready for a proper review now.

[ikudrin]

The patch should probably be split into smaller, more focused ones. DWARFDataExtractor::getInitialLength() and its test should go first, and then distinct patches for each debug section.

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

45

It looks like that behavior is not stated. I hardly can remember if I saw it was used anywhere, but once I fixed an issue which was (partially) caused by it, D71704. I guess we can easily drop it if that helps to simplify the code, maybe adding a warning note in the comments of the methods.

[jhenderson]

As suggested, please split this up into separate patches.

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

43

Perhaps a comment explaining what is meant by an initial length field, wouldn't be out-of-order?

llvm/lib/DebugInfo/DWARF/DWARFDataExtractor.cpp

31

I think it's more common for error/warning messages to use lower-case for their first word.

llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test

53 ↗

(On Diff #244652)

I don't think this is a good change in behaviour. We should still be able to dump the total length field, with the reserved value.

195 ↗

(On Diff #244652)

This is a regression in the quality of the error, as it is not clear what section this error comes from. Same goes for many of the other changes in errors.

llvm/test/tools/llvm-dwarfdump/X86/debug_rnglists_invalid.s

5 ↗

(On Diff #244652)

This error is basically useless. Where? What section? What does "unexpected end of data" mean?

llvm/test/tools/llvm-dwarfdump/X86/debug_rnglists_reserved_length.s

5 ↗

(On Diff #244652)

Same as above for .debug_line.

llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp

43

Ideally, we should check the error returned in this and similar cases, to show that it is what was expected.

46

It may be worth a brief comment before each pair of checks to show what the intent of the particularly case is.

49–53

Will this test work if the host endianness is different? It looks to me like 0x00010203 != {0x00, 0x01, 0x02, 0x03} for little endian hosts, since in that case, 0x00010203 is {0x03, 0x02, 0x01, 0x00}.

67

clang-format?

[labath]

Sorry about the delay (and thank you for the prompt response). It took a while before this got to the top of my stack.

I'll split this patch up with my next update, which will come when I figure out how to improve the error messages. My initial motivation for putting this into a single patch was to make the impact of this kind of change more obvious (I expected the change in error messages to be mildly controversial), but in retrospect, that could have also been achieved with separate linked patches. Having super-descriptive error messages is somewhat at odds with making the code reusable, but I think we can come up with a reasonable compromise.

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

45

This behavior is stated (and painstakingly reiterated) on every DataExtractor method. It looks like DWARFDataExtractor does not have these comments, but then this class is much more lightly documented. Nonetheless, I think it would be good to preserve this behavior, as a matter of consistency.

llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test

53 ↗

(On Diff #244652)

Actually, I would say that here we should not print out the prologue at all (just some error about invalid initial length). Since the length field is not valid, it's hard to say whether we have any line table here at all -- and yet we will print out the whole prologue full of zeroes (even before this patch).

195 ↗

(On Diff #244652)

When this is run normally on the command line (so both stdout and stderr go to the terminal), this comes out as:

debug_line[0x00000048]
warning: Unsupported reserved unit length of value 0xfffffffe at offset 0x48
...

So, the section where this comes from should be fairly obvious. Nonetheless, it should be pretty easy to prepend the error message with the section name. Maybe something like "warning: parsing line table prologue at offset 0x00000048: unsupported reserved unit length found of value 0xfffffffe" or "warning: parsing line table prologue: unsupported reserved unit length of value 0xfffffffe at offset 0x48" ?

The question boils down to whether it should be the DataExtractor who embeds the offset into the message, or the line table code.

[ikudrin]

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

45

Checked again and now I see it; you are right, it is indeed stated. In that case, I agree, it is better to stick with it.

[jhenderson]

llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test

195 ↗

(On Diff #244652)

I don't have an answer on what layer adds the offset context to the message, as I don't really have any particular opinion on that. However, it's worth noting that other tools use this code (e.g. LLD), and they aren't printing the debug_line[0x00000048] or indeed any of the rest of the dumped output, only the warning/error. As such, without the offset context, it's impossible to see the problem from those tools' output.

[labath]

Thanks for all the feedback. I've removed all changes to individual parsing code
from this patch. Will post those as separate patches.

This change now only deals with the new API.

[jhenderson]

LGTM.

llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp

12

Nit: blank line before the using please.

58

is hard -> it is hard

[labath]

llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp

49–53

(I thought I replied to this already, but I don't see the comment anywhere.)

The test hardcodes a big-endian so this will work regardless of the host. I used big endian because the numbers come out more naturally that way, and because they are more likely to flesh out any host endiannes creeping in.

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

 //===- DWARFDataExtractor.h -------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_DEBUGINFO_DWARFDATAEXTRACTOR_H
 #define LLVM_DEBUGINFO_DWARFDATAEXTRACTOR_H
 
+#include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/DebugInfo/DWARF/DWARFSection.h"
 #include "llvm/Support/DataExtractor.h"
 
 namespace llvm {
 class DWARFObject;
 
 /// A DataExtractor (typically for an in-memory copy of an object-file section)
 /// plus a relocation map for that section, if there is one.
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) {}
 
+  /// 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;
+
+  std::pair<uint64_t, dwarf::DwarfFormat> getInitialLength(Cursor &C) const {
+    return getInitialLength(&getOffset(C), &getError(C));
+  }
+
   /// Extracts a value and applies a relocation to the result if
   /// one exists for the given offset.
   uint64_t getRelocatedValue(uint32_t Size, uint64_t *Off,
                              uint64_t *SectionIndex = nullptr,
                              Error *Err = nullptr) const;
+  uint64_t getRelocatedValue(Cursor &C, uint32_t Size,
+                             uint64_t *SectionIndex = nullptr) const {
+    return getRelocatedValue(Size, &getOffset(C), SectionIndex, &getError(C));
+  }
 
   /// Extracts an address-sized value and applies a relocation to the result if
   /// one exists for the given offset.
   uint64_t getRelocatedAddress(uint64_t *Off, uint64_t *SecIx = nullptr) const {
     return getRelocatedValue(getAddressSize(), Off, SecIx);
   }
   uint64_t getRelocatedAddress(Cursor &C, uint64_t *SecIx = nullptr) const {
     return getRelocatedValue(getAddressSize(), &getOffset(C), SecIx,

llvm/lib/DebugInfo/DWARF/DWARFDataExtractor.cpp

 //===- DWARFDataExtractor.cpp ---------------------------------------------===//
 //
 // 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/DebugInfo/DWARF/DWARFDataExtractor.h"
-#include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
 
 using namespace llvm;
 
+std::pair<uint64_t, dwarf::DwarfFormat>
+DWARFDataExtractor::getInitialLength(uint64_t *Off, Error *Err) const {
+  ErrorAsOutParameter ErrAsOut(Err);
+  if (Err && *Err)
+    return {0, dwarf::DWARF32};
+
+  Cursor C(*Off);
+  uint64_t Length = getRelocatedValue(C, 4);
+  dwarf::DwarfFormat Format = dwarf::DWARF32;
+  if (Length == dwarf::DW_LENGTH_DWARF64) {
+    Length = getRelocatedValue(C, 8);
+    Format = dwarf::DWARF64;
+  } else if (Length >= dwarf::DW_LENGTH_lo_reserved) {
+    cantFail(C.takeError());
+    if (Err)
+      *Err = createStringError(
+          errc::invalid_argument,
+          "unsupported reserved unit length of value 0x%8.8" PRIx64, Length);
+    return {0, dwarf::DWARF32};
+  }
+
+  if (C) {
+    *Off = C.tell();
+    return {Length, Format};
+  }
+  if (Err)
+    *Err = C.takeError();
+  else
+    consumeError(C.takeError());
+  return {0, dwarf::DWARF32};
+}
+
 uint64_t DWARFDataExtractor::getRelocatedValue(uint32_t Size, uint64_t *Off,
                                                uint64_t *SecNdx,
                                                Error *Err) const {
   if (SecNdx)
     *SecNdx = object::SectionedAddress::UndefSection;
   if (!Section)
     return getUnsigned(Off, Size, Err);
   Optional<RelocAddrEntry> E = Obj->find(*Section, *Off);

llvm/unittests/DebugInfo/DWARF/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   ${LLVM_TARGETS_TO_BUILD}
   AsmPrinter
   DebugInfoDWARF
   MC
   Object
   ObjectYAML
   Support
   )
 
 add_llvm_unittest(DebugInfoDWARFTests
   DwarfGenerator.cpp
   DwarfUtils.cpp
   DWARFAcceleratorTableTest.cpp
+  DWARFDataExtractorTest.cpp
   DWARFDebugArangeSetTest.cpp
   DWARFDebugInfoTest.cpp
   DWARFDebugLineTest.cpp
   DWARFDieTest.cpp
   DWARFFormValueTest.cpp
   DWARFLocationExpressionTest.cpp
   )
 
 target_link_libraries(DebugInfoDWARFTests PRIVATE LLVMTestingSupport)

llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp

+//===- DWARFDataExtractorTest.cpp -----------------------------------------===//
+//
+// 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/DebugInfo/DWARF/DWARFDataExtractor.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+using namespace llvm;
+
+namespace {
+
+TEST(DWARFDataExtractorTest, getInitialLength) {
+  auto GetWithError = [](ArrayRef<uint8_t> Bytes)
+      -> Expected<std::tuple<uint64_t, dwarf::DwarfFormat, uint64_t>> {
+    DWARFDataExtractor Data(Bytes, /*IsLittleEndian=*/false, /*AddressSize=*/8);
+    DWARFDataExtractor::Cursor C(0);
+    uint64_t Length;
+    dwarf::DwarfFormat Format;
+    std::tie(Length, Format) = Data.getInitialLength(C);
+    if (C)
+      return std::make_tuple(Length, Format, C.tell());
+
+    EXPECT_EQ(Length, 0u);
+    EXPECT_EQ(Format, dwarf::DWARF32);
+    EXPECT_EQ(C.tell(), 0u);
+    return C.takeError();
+  };
+  auto GetWithoutError = [](ArrayRef<uint8_t> Bytes) {
+    DWARFDataExtractor Data(Bytes, /*IsLittleEndian=*/false, /*AddressSize=*/8);
+    uint64_t Offset = 0;
+    uint64_t Length;
+    dwarf::DwarfFormat Format;
+    std::tie(Length, Format) = Data.getInitialLength(&Offset);
+    return std::make_tuple(Length, Format, Offset);
+  };
+  auto ErrorResult = std::make_tuple(0, dwarf::DWARF32, 0);
+
+  // Empty data.
+  EXPECT_THAT_EXPECTED(GetWithError({}),
+                       FailedWithMessage("unexpected end of data"));
+  EXPECT_EQ(GetWithoutError({}), ErrorResult);
+
+  // Not long enough for the U32 field.
+  EXPECT_THAT_EXPECTED(GetWithError({0x00, 0x01, 0x02}),
+                       FailedWithMessage("unexpected end of data"));
+  EXPECT_EQ(GetWithoutError({0x00, 0x01, 0x02}), ErrorResult);
+
+  EXPECT_THAT_EXPECTED(
+      GetWithError({0x00, 0x01, 0x02, 0x03}),
+      HasValue(std::make_tuple(0x00010203, dwarf::DWARF32, 4)));
+  EXPECT_EQ(GetWithoutError({0x00, 0x01, 0x02, 0x03}),
+            std::make_tuple(0x00010203, dwarf::DWARF32, 4));
+
+  // Zeroes are not an error, but without the Error object is hard to tell them
+  // apart from a failed read.
+  EXPECT_THAT_EXPECTED(
+      GetWithError({0x00, 0x00, 0x00, 0x00}),
+      HasValue(std::make_tuple(0x00000000, dwarf::DWARF32, 4)));
+  EXPECT_EQ(GetWithoutError({0x00, 0x00, 0x00, 0x00}),
+            std::make_tuple(0x00000000, dwarf::DWARF32, 4));
+
+  // Smallest invalid value.
+  EXPECT_THAT_EXPECTED(
+      GetWithError({0xff, 0xff, 0xff, 0xf0}),
+      FailedWithMessage(
+          "unsupported reserved unit length of value 0xfffffff0"));
+  EXPECT_EQ(GetWithoutError({0xff, 0xff, 0xff, 0xf0}), ErrorResult);
+
+  // DWARF64 marker without the subsequent length field.
+  EXPECT_THAT_EXPECTED(GetWithError({0xff, 0xff, 0xff, 0xff}),
+                       FailedWithMessage("unexpected end of data"));
+  EXPECT_EQ(GetWithoutError({0xff, 0xff, 0xff, 0xff}), ErrorResult);
+
+  // Not enough data for the U64 length.
+  EXPECT_THAT_EXPECTED(
+      GetWithError({0xff, 0xff, 0xff, 0xff, 0x00, 0x01, 0x02, 0x03}),
+      FailedWithMessage("unexpected end of data"));
+  EXPECT_EQ(GetWithoutError({0xff, 0xff, 0xff, 0xff, 0x00, 0x01, 0x02, 0x03}),
+            ErrorResult);
+
+  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));
+}
+
+} // namespace