[Support] Make DataExtractor error messages more clear
authorPavel Labath <pavel@labath.sk>
Mon, 20 Apr 2020 15:28:15 +0000 (17:28 +0200)
committerPavel Labath <pavel@labath.sk>
Tue, 2 Jun 2020 10:57:51 +0000 (12:57 +0200)
Summary:
This is a result of the discussion at D78113. Previously we would be
only giving the current offset at which the error was detected. However,
this was phrased somewhat ambiguously (as it could also mean that end of
data was at that offset). The new error message includes the current
offset as well as the extent of the data being read.

I've changed a couple of file-level static functions into private member
functions in order to avoid passing a bunch of new arguments everywhere.

Reviewers: dblaikie, jhenderson

Subscribers: hiraditya, MaskRay, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D78558

13 files changed:
llvm/include/llvm/Support/DataExtractor.h
llvm/lib/Support/DataExtractor.cpp
llvm/test/DebugInfo/X86/dwarfdump-debug-loc-error-cases2.s
llvm/test/DebugInfo/X86/dwarfdump-debug-loclists-error-cases2.s
llvm/test/tools/llvm-dwarfdump/X86/debug_addr_too_small_for_extended_length_field.s
llvm/test/tools/llvm-dwarfdump/X86/debug_addr_too_small_for_length_field.s
llvm/test/tools/llvm-dwarfdump/X86/debug_rnglists_invalid.s
llvm/unittests/DebugInfo/DWARF/DWARFAcceleratorTableTest.cpp
llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp
llvm/unittests/DebugInfo/DWARF/DWARFDebugArangeSetTest.cpp
llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
llvm/unittests/DebugInfo/DWARF/DWARFDieTest.cpp
llvm/unittests/Support/DataExtractorTest.cpp

index eb0edd1..f9335c1 100644 (file)
@@ -689,6 +689,16 @@ protected:
   // public.
   static uint64_t &getOffset(Cursor &C) { return C.Offset; }
   static Error &getError(Cursor &C) { return C.Err; }
+
+private:
+  /// If it is possible to read \a Size bytes at offset \a Offset, returns \b
+  /// true. Otherwise, returns \b false. If \a E is not nullptr, also sets the
+  /// error object to indicate an error.
+  bool prepareRead(uint64_t Offset, uint64_t Size, Error *E) const;
+
+  template <typename T> T getU(uint64_t *OffsetPtr, Error *Err) const;
+  template <typename T>
+  T *getUs(uint64_t *OffsetPtr, T *Dst, uint32_t Count, Error *Err) const;
 };
 
 } // namespace llvm
index 99265c4..133d674 100644 (file)
 
 using namespace llvm;
 
-static void unexpectedEndReached(Error *E, uint64_t Offset) {
-  if (E)
-    *E = createStringError(errc::illegal_byte_sequence,
-                           "unexpected end of data at offset 0x%" PRIx64,
-                           Offset);
+bool DataExtractor::prepareRead(uint64_t Offset, uint64_t Size,
+                                Error *E) const {
+  if (isValidOffsetForDataOfSize(Offset, Size))
+    return true;
+  if (E) {
+    if (Offset <= Data.size())
+      *E = createStringError(
+          errc::illegal_byte_sequence,
+          "unexpected end of data at offset 0x%zx while reading [0x%" PRIx64
+          ", 0x%" PRIx64 ")",
+          Data.size(), Offset, Offset + Size);
+    else
+      *E = createStringError(errc::invalid_argument,
+                             "offset 0x%" PRIx64
+                             " is beyond the end of data at 0x%zx",
+                             Offset, Data.size());
+  }
+  return false;
 }
 
 static bool isError(Error *E) { return E && *E; }
 
 template <typename T>
-static T getU(uint64_t *offset_ptr, const DataExtractor *de,
-              bool isLittleEndian, const char *Data, llvm::Error *Err) {
+T DataExtractor::getU(uint64_t *offset_ptr, Error *Err) const {
   ErrorAsOutParameter ErrAsOut(Err);
   T val = 0;
   if (isError(Err))
     return val;
 
   uint64_t offset = *offset_ptr;
-  if (!de->isValidOffsetForDataOfSize(offset, sizeof(T))) {
-    unexpectedEndReached(Err, offset);
+  if (!prepareRead(offset, sizeof(T), Err))
     return val;
-  }
-  std::memcpy(&val, &Data[offset], sizeof(val));
-  if (sys::IsLittleEndianHost != isLittleEndian)
+  std::memcpy(&val, &Data.data()[offset], sizeof(val));
+  if (sys::IsLittleEndianHost != IsLittleEndian)
     sys::swapByteOrder(val);
 
   // Advance the offset
@@ -47,22 +57,19 @@ static T getU(uint64_t *offset_ptr, const DataExtractor *de,
 }
 
 template <typename T>
-static T *getUs(uint64_t *offset_ptr, T *dst, uint32_t count,
-                const DataExtractor *de, bool isLittleEndian, const char *Data,
-                llvm::Error *Err) {
+T *DataExtractor::getUs(uint64_t *offset_ptr, T *dst, uint32_t count,
+                        Error *Err) const {
   ErrorAsOutParameter ErrAsOut(Err);
   if (isError(Err))
     return nullptr;
 
   uint64_t offset = *offset_ptr;
 
-  if (!de->isValidOffsetForDataOfSize(offset, sizeof(*dst) * count)) {
-    unexpectedEndReached(Err, offset);
+  if (!prepareRead(offset, sizeof(*dst) * count, Err))
     return nullptr;
-  }
   for (T *value_ptr = dst, *end = dst + count; value_ptr != end;
        ++value_ptr, offset += sizeof(*dst))
-    *value_ptr = getU<T>(offset_ptr, de, isLittleEndian, Data, Err);
+    *value_ptr = getU<T>(offset_ptr, Err);
   // Advance the offset
   *offset_ptr = offset;
   // Return a non-NULL pointer to the converted data as an indicator of
@@ -71,55 +78,49 @@ static T *getUs(uint64_t *offset_ptr, T *dst, uint32_t count,
 }
 
 uint8_t DataExtractor::getU8(uint64_t *offset_ptr, llvm::Error *Err) const {
-  return getU<uint8_t>(offset_ptr, this, IsLittleEndian, Data.data(), Err);
+  return getU<uint8_t>(offset_ptr, Err);
 }
 
-uint8_t *
-DataExtractor::getU8(uint64_t *offset_ptr, uint8_t *dst, uint32_t count) const {
-  return getUs<uint8_t>(offset_ptr, dst, count, this, IsLittleEndian,
-                        Data.data(), nullptr);
+uint8_t *DataExtractor::getU8(uint64_t *offset_ptr, uint8_t *dst,
+                              uint32_t count) const {
+  return getUs<uint8_t>(offset_ptr, dst, count, nullptr);
 }
 
 uint8_t *DataExtractor::getU8(Cursor &C, uint8_t *Dst, uint32_t Count) const {
-  return getUs<uint8_t>(&C.Offset, Dst, Count, this, IsLittleEndian,
-                        Data.data(), &C.Err);
+  return getUs<uint8_t>(&C.Offset, Dst, Count, &C.Err);
 }
 
 uint16_t DataExtractor::getU16(uint64_t *offset_ptr, llvm::Error *Err) const {
-  return getU<uint16_t>(offset_ptr, this, IsLittleEndian, Data.data(), Err);
+  return getU<uint16_t>(offset_ptr, Err);
 }
 
 uint16_t *DataExtractor::getU16(uint64_t *offset_ptr, uint16_t *dst,
                                 uint32_t count) const {
-  return getUs<uint16_t>(offset_ptr, dst, count, this, IsLittleEndian,
-                         Data.data(), nullptr);
+  return getUs<uint16_t>(offset_ptr, dst, count, nullptr);
 }
 
 uint32_t DataExtractor::getU24(uint64_t *OffsetPtr, Error *Err) const {
-  uint24_t ExtractedVal =
-      getU<uint24_t>(OffsetPtr, this, IsLittleEndian, Data.data(), Err);
+  uint24_t ExtractedVal = getU<uint24_t>(OffsetPtr, Err);
   // The 3 bytes are in the correct byte order for the host.
   return ExtractedVal.getAsUint32(sys::IsLittleEndianHost);
 }
 
 uint32_t DataExtractor::getU32(uint64_t *offset_ptr, llvm::Error *Err) const {
-  return getU<uint32_t>(offset_ptr, this, IsLittleEndian, Data.data(), Err);
+  return getU<uint32_t>(offset_ptr, Err);
 }
 
 uint32_t *DataExtractor::getU32(uint64_t *offset_ptr, uint32_t *dst,
                                 uint32_t count) const {
-  return getUs<uint32_t>(offset_ptr, dst, count, this, IsLittleEndian,
-                         Data.data(), nullptr);
+  return getUs<uint32_t>(offset_ptr, dst, count, nullptr);
 }
 
 uint64_t DataExtractor::getU64(uint64_t *offset_ptr, llvm::Error *Err) const {
-  return getU<uint64_t>(offset_ptr, this, IsLittleEndian, Data.data(), Err);
+  return getU<uint64_t>(offset_ptr, Err);
 }
 
 uint64_t *DataExtractor::getU64(uint64_t *offset_ptr, uint64_t *dst,
                                 uint32_t count) const {
-  return getUs<uint64_t>(offset_ptr, dst, count, this, IsLittleEndian,
-                         Data.data(), nullptr);
+  return getUs<uint64_t>(offset_ptr, dst, count, nullptr);
 }
 
 uint64_t DataExtractor::getUnsigned(uint64_t *offset_ptr, uint32_t byte_size,
@@ -163,7 +164,10 @@ StringRef DataExtractor::getCStrRef(uint64_t *OffsetPtr, Error *Err) const {
     *OffsetPtr = Pos + 1;
     return StringRef(Data.data() + Start, Pos - Start);
   }
-  unexpectedEndReached(Err, Start);
+  if (Err)
+    *Err = createStringError(errc::illegal_byte_sequence,
+                             "no null terminated string at offset 0x%" PRIx64,
+                             Start);
   return StringRef();
 }
 
@@ -180,10 +184,8 @@ StringRef DataExtractor::getBytes(uint64_t *OffsetPtr, uint64_t Length,
   if (isError(Err))
     return StringRef();
 
-  if (!isValidOffsetForDataOfSize(*OffsetPtr, Length)) {
-    unexpectedEndReached(Err, *OffsetPtr);
+  if (!prepareRead(*OffsetPtr, Length, Err))
     return StringRef();
-  }
 
   StringRef Result = Data.substr(*OffsetPtr, Length);
   *OffsetPtr += Length;
@@ -229,8 +231,6 @@ void DataExtractor::skip(Cursor &C, uint64_t Length) const {
   if (isError(&C.Err))
     return;
 
-  if (isValidOffsetForDataOfSize(C.Offset, Length))
+  if (prepareRead(C.Offset, Length, &C.Err))
     C.Offset += Length;
-  else
-    unexpectedEndReached(&C.Err, C.Offset);
 }
index 8a1abd6..b6eae6f 100644 (file)
@@ -9,11 +9,11 @@
 
 # CHECK:      DW_AT_name        ("x1")
 # CHECK-NEXT: DW_AT_location    (0xdeadbeef: )
-# ERR:    error: unexpected end of data at offset 0xdeadbeef
+# ERR:    error: offset 0xdeadbeef is beyond the end of data at 0x48
 
 # CHECK:      DW_AT_name        ("x2")
 # CHECK-NEXT: DW_AT_location    (0x00000036: )
-# ERR:    error: unexpected end of data at offset 0x48
+# ERR:    error: unexpected end of data at offset 0x48 while reading [0x48, 0xdef5)
 
 
         .type   f,@function
index 14dc7f4..39bbeeb 100644 (file)
@@ -9,11 +9,11 @@
 
 # CHECK:      DW_AT_name        ("x1")
 # CHECK-NEXT: DW_AT_location    (0xdeadbeef: )
-# ERR:    error: unexpected end of data at offset 0xdeadbeef
+# ERR:    error: offset 0xdeadbeef is beyond the end of data at 0x34
 
 # CHECK:      DW_AT_name        ("x2")
 # CHECK-NEXT: DW_AT_location    (0x00000025
-# ERR:    error: unexpected end of data at offset 0x34
+# ERR:    error: unexpected end of data at offset 0x34 while reading [0x34, 0xdeadbf23)
 
 
         .type   f,@function
index 1034c97..74f1e09 100644 (file)
@@ -4,7 +4,7 @@
 
 # CHECK: .debug_addr contents:
 # CHECK-NOT: {{.}}
-# ERR: parsing address table at offset 0x0: unexpected end of data at offset 0x4
+# ERR: parsing address table at offset 0x0: unexpected end of data at offset 0xb while reading [0x4, 0xc)
 # ERR-NOT: {{.}}
 
 # too small section to contain an extended length field of a DWARF64 address table.
index bc840eb..7277449 100644 (file)
@@ -4,7 +4,7 @@
 
 # CHECK: .debug_addr contents:
 # CHECK-NOT: {{.}}
-# ERR: parsing address table at offset 0x0: unexpected end of data at offset 0x0
+# ERR: parsing address table at offset 0x0: unexpected end of data at offset 0x2 while reading [0x0, 0x4)
 # ERR-NOT: {{.}}
 
 # too small section to contain length field
index d176363..ad4d7dd 100644 (file)
@@ -2,7 +2,7 @@
 # RUN: not llvm-dwarfdump --debug-rnglists - 2>&1 | FileCheck %s --check-prefix=SHORT
 # SHORT-NOT: error:
 # SHORT-NOT: range list header
-# SHORT: error: parsing .debug_rnglists table at offset 0x0: unexpected end of data at offset 0x0
+# SHORT: error: parsing .debug_rnglists table at offset 0x0: unexpected end of data at offset 0x3
 # SHORT-NOT: range list header
 # SHORT-NOT: error:
 
index 0d68083..38f3e07 100644 (file)
@@ -43,7 +43,7 @@ TEST(DWARFDebugNames, TooSmallForDWARF64) {
       ExtractDebugNames(StringRef(NamesSecData, sizeof(NamesSecData)),
                         StringRef()),
       FailedWithMessage("parsing .debug_names header at 0x0: unexpected end of "
-                        "data at offset 0x28"));
+                        "data at offset 0x2b while reading [0x28, 0x2c)"));
 }
 
 } // end anonymous namespace
index d6b13c7..449442f 100644 (file)
@@ -61,8 +61,10 @@ Symbols:
   DataExtractor::Cursor C(0);
   EXPECT_EQ(0x42u, Data.getRelocatedAddress(C));
   EXPECT_EQ(0u, Data.getRelocatedAddress(C));
-  EXPECT_THAT_ERROR(C.takeError(),
-                    FailedWithMessage("unexpected end of data at offset 0x4"));
+  EXPECT_THAT_ERROR(
+      C.takeError(),
+      FailedWithMessage(
+          "unexpected end of data at offset 0x6 while reading [0x4, 0x8)"));
 }
 
 TEST(DWARFDataExtractorTest, getInitialLength) {
@@ -94,13 +96,15 @@ TEST(DWARFDataExtractorTest, getInitialLength) {
   // Empty data.
   EXPECT_THAT_EXPECTED(
       GetWithError({}),
-      FailedWithMessage("unexpected end of data at offset 0x0"));
+      FailedWithMessage(
+          "unexpected end of data at offset 0x0 while reading [0x0, 0x4)"));
   EXPECT_EQ(GetWithoutError({}), ErrorResult);
 
   // Not long enough for the U32 field.
   EXPECT_THAT_EXPECTED(
       GetWithError({0x00, 0x01, 0x02}),
-      FailedWithMessage("unexpected end of data at offset 0x0"));
+      FailedWithMessage(
+          "unexpected end of data at offset 0x3 while reading [0x0, 0x4)"));
   EXPECT_EQ(GetWithoutError({0x00, 0x01, 0x02}), ErrorResult);
 
   EXPECT_THAT_EXPECTED(
@@ -127,13 +131,15 @@ TEST(DWARFDataExtractorTest, getInitialLength) {
   // DWARF64 marker without the subsequent length field.
   EXPECT_THAT_EXPECTED(
       GetWithError({0xff, 0xff, 0xff, 0xff}),
-      FailedWithMessage("unexpected end of data at offset 0x4"));
+      FailedWithMessage(
+          "unexpected end of data at offset 0x4 while reading [0x4, 0xc)"));
   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 at offset 0x4"));
+      FailedWithMessage(
+          "unexpected end of data at offset 0x8 while reading [0x4, 0xc)"));
   EXPECT_EQ(GetWithoutError({0xff, 0xff, 0xff, 0xff, 0x00, 0x01, 0x02, 0x03}),
             ErrorResult);
 
@@ -195,21 +201,27 @@ Symbols:
   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"));
+  EXPECT_THAT_ERROR(
+      C.takeError(),
+      FailedWithMessage(
+          "unexpected end of data at offset 0x8 while reading [0x8, 0xc)"));
 
   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"));
+  EXPECT_THAT_ERROR(
+      C.takeError(),
+      FailedWithMessage(
+          "unexpected end of data at offset 0x6 while reading [0x4, 0x8)"));
 
   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"));
+  EXPECT_THAT_ERROR(
+      C.takeError(),
+      FailedWithMessage(
+          "unexpected end of data at offset 0x2 while reading [0x0, 0x4)"));
 }
 
 } // namespace
index 5629a0d..4ec9c5d 100644 (file)
@@ -121,7 +121,7 @@ TEST(DWARFDebugArangeSet, SectionTooShort) {
   static const char DebugArangesSecRaw[11 + 1] = {0};
   ExpectExtractError(DebugArangesSecRaw,
                      "parsing address ranges table at offset 0x0: unexpected "
-                     "end of data at offset 0xb");
+                     "end of data at offset 0xb while reading [0xb, 0xc)");
 }
 
 TEST(DWARFDebugArangeSet, SectionTooShortDWARF64) {
@@ -130,7 +130,7 @@ TEST(DWARFDebugArangeSet, SectionTooShortDWARF64) {
       "\xff\xff\xff\xff"; // DWARF64 mark
   ExpectExtractError(DebugArangesSecRaw,
                      "parsing address ranges table at offset 0x0: unexpected "
-                     "end of data at offset 0x17");
+                     "end of data at offset 0x17 while reading [0x17, 0x18)");
 }
 
 TEST(DWARFDebugArangeSet, NoSpaceForEntries) {
index be61c03..7ceb656 100644 (file)
@@ -211,8 +211,9 @@ TEST_F(DebugLineBasicFixture, GetOrParseLineTableAtInvalidOffsetAfterData) {
 
   EXPECT_THAT_EXPECTED(
       getOrParseLineTableFatalErrors(0),
-      FailedWithMessage("parsing line table prologue at offset 0x00000000: "
-                        "unexpected end of data at offset 0x0"));
+      FailedWithMessage(
+          "parsing line table prologue at offset 0x00000000: "
+          "unexpected end of data at offset 0x1 while reading [0x0, 0x4)"));
 
   EXPECT_THAT_EXPECTED(
       getOrParseLineTableFatalErrors(1),
index b662965..ad00537 100644 (file)
@@ -107,7 +107,8 @@ TEST(DWARFDie, getLocations) {
 
   EXPECT_THAT_EXPECTED(
       Die.getLocations(DW_AT_call_data_location),
-      FailedWithMessage("unexpected end of data at offset 0x20"));
+      FailedWithMessage(
+          "unexpected end of data at offset 0x20 while reading [0x20, 0x21)"));
 
   EXPECT_THAT_EXPECTED(
       Die.getLocations(DW_AT_call_data_value),
index cec0855..278e588 100644 (file)
@@ -102,8 +102,9 @@ TEST(DataExtractorTest, Strings) {
   EXPECT_EQ(11U, C.tell());
   EXPECT_EQ(nullptr, DE.getCStr(C));
   EXPECT_EQ(11U, C.tell());
-  EXPECT_THAT_ERROR(C.takeError(),
-                    FailedWithMessage("unexpected end of data at offset 0xb"));
+  EXPECT_THAT_ERROR(
+      C.takeError(),
+      FailedWithMessage("no null terminated string at offset 0xb"));
 }
 
 TEST(DataExtractorTest, LEB128) {
@@ -270,6 +271,12 @@ TEST(DataExtractorTest, getU8_vector) {
   DE.getU8(C, S, 2);
   EXPECT_THAT_ERROR(C.takeError(), Succeeded());
   EXPECT_EQ("AB", toStringRef(S));
+
+  C = DataExtractor::Cursor(0x47);
+  DE.getU8(C, S, 2);
+  EXPECT_THAT_ERROR(
+      C.takeError(),
+      FailedWithMessage("offset 0x47 is beyond the end of data at 0x2"));
 }
 
 TEST(DataExtractorTest, getU24) {