[DWARF] Return Error from DWARFDebugArangeSet::extract().

[ikudrin]

Summary

This helps to detect and report parsing errors better. The patch follows the ideas of LLDB's patches D59370 and D59381.

It adds tests for valid and some invalid cases. More checks and tests to come. Note that the patch fixes validation of the Length field because the value does not include the field itself.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp

85

In a separate patch, might be good to add error handling to stop when the Header length is reached, rather than when the section end is reached.

91

I'd probably write these ays "x == 0" rather than "!x"

91–92

Potentially another error case could be added here (in another patch) in the case where the list is terminated earlier than the Length specified in the header.

96

Looks like the old code fails if the range list is empty - and maybe the new code deosn't fail in that case? Worth adding a test case? (it actually seems reasonable to me to support it - you could imagine a CU with only type information in it & so no covered addresses, and having an arange entry describing that so the consumer didn't have to go discover it through parsing the CU, etc)

[ikudrin]

• "!x" -> "x == 0";
• Improved error messages wordings;
• Added more tests;
• Fixed checking the value of the Length field.

[ikudrin]

Thanks for the suggestions, @dblaikie! I've added D71931 and D71932 with corresponding patches.

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp

60–63

Could this be phrased more specifically - "length exceeds section size" or something like that?

64–68

Could this be more specific about what's invalid about the address size ("unsupported address size: 5 (4 and 8 supported)" or the like?)

[clayborg]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

458

does "toString()" consume the error?

llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp

34

return error instead of crashing the program

llvm/lib/DebugInfo/DWARF/DWARFDebugAranges.cpp

23

return llvm::Error here?

29–31

return an error if Set.extract fails? Seems weird to add error handling to only convert it to bool and ignore the error?

llvm/tools/obj2yaml/dwarf2yaml.cpp

65–67

Dump the error if Set.extract() fails?

[ikudrin]

• Update wordings.
• Replace most of the lit tests to check error cases with gtest-based ones.
• Do not silently absorb the error messages.
• Update existing tests and add new ones to reflect the changes.

[ikudrin]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

458

Yes. It uses handleAllErrors() internally.

llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp

34

This just checks for the precondition. Callers are responsible to call the method with valid arguments. Therefore, if this triggers, it should be considered as an internal error and fixed.

llvm/lib/DebugInfo/DWARF/DWARFDebugAranges.cpp

23

As this is just a private helper method for DWARFDebugAranges::generate(), I opted to print the errors here.

llvm/tools/obj2yaml/dwarf2yaml.cpp

65–67

Done, but it pulled a lot of changes. Maybe it worth to move all them into a separate patch.

[clayborg]

lgtm with Error being returned everywhere now.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

456–461

do we want {} on this while now?

[ikudrin]

• Used curly brackets for the while loop in dumping .debug_aranges in DWARFContext::dump().
• Removed else branch after break.

Thanks, @clayborg!

[dblaikie]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

456–459

Might this eventually want to go in the direction of having fallible errors and errors that can continue, like some of @jhenderson 's work? Should we come up with a consistent strategy for this sort of thing as it seems we've developed a few different error handling parsing schemes at this point.

[jhenderson]

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

456–459

Yeah, we should a) be consistent (though I don't mind specifically what we standardise on) and b) avoid printing such low-level errors in a library, as it makes things hard for clients. See my lightning talk on the subject that was inspired by my debug line error handling work some time ago: https://www.youtube.com/watch?v=YSEY4pg1YB0

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugArangeSet.h

 //===- DWARFDebugArangeSet.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_DWARFDEBUGARANGESET_H
 #define LLVM_DEBUGINFO_DWARFDEBUGARANGESET_H
 
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/DataExtractor.h"
+#include "llvm/Support/Error.h"
 #include <cstdint>
 #include <vector>
 
 namespace llvm {
 
 class raw_ostream;
 
 class DWARFDebugArangeSet {
private:
   uint64_t Offset;
   Header HeaderData;
   DescriptorColl ArangeDescriptors;
 
 public:
   DWARFDebugArangeSet() { clear(); }
 
   void clear();
-  bool extract(DataExtractor data, uint64_t *offset_ptr);
+  Error extract(DataExtractor data, uint64_t *offset_ptr);
   void dump(raw_ostream &OS) const;
 
   uint32_t getCompileUnitDIEOffset() const { return HeaderData.CuOffset; }
 
   const Header &getHeader() const { return HeaderData; }
 
   desc_iterator_range descriptors() const {
     return desc_iterator_range(ArangeDescriptors.begin(),
                                ArangeDescriptors.end());
   }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARFDEBUGARANGESET_H

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp

if (DumpType & DIDT_DebugMacro) {
     }
   }
 
   if (shouldDump(Explicit, ".debug_aranges", DIDT_ID_DebugAranges,
                  DObj->getArangesSection())) {
     uint64_t offset = 0;
     DataExtractor arangesData(DObj->getArangesSection(), isLittleEndian(), 0);
     DWARFDebugArangeSet set;
-    while (set.extract(arangesData, &offset))
+    while (arangesData.isValidOffset(offset))
+      if (Error E = set.extract(arangesData, &offset)) {
+        WithColor::error() << toString(std::move(E)) << '\n';
+        break;
+      } else
         set.dump(OS);
   }
 
   auto DumpLineSection = [&](DWARFDebugLine::SectionParser Parser,
                              DIDumpOptions DumpOpts,
                              Optional<uint64_t> DumpOffset) {
     while (!Parser.done()) {
       if (DumpOffset && Parser.getOffset() != *DumpOffset) {
         Parser.skip(dumpWarning);

llvm/lib/DebugInfo/DWARF/DWARFDebugArangeSet.cpp

 //===- DWARFDebugArangeSet.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/DWARFDebugArangeSet.h"
+#include "llvm/Support/Errc.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
 #include <cinttypes>
 #include <cstdint>
 #include <cstring>
 
 using namespace llvm;
 
 void DWARFDebugArangeSet::Descriptor::dump(raw_ostream &OS,
                                            uint32_t AddressSize) const {
   OS << format("[0x%*.*" PRIx64 ", ", AddressSize * 2, AddressSize * 2, Address)
      << format(" 0x%*.*" PRIx64 ")", AddressSize * 2, AddressSize * 2,
                getEndAddress());
 }
 
 void DWARFDebugArangeSet::clear() {
   Offset = -1ULL;
   std::memset(&HeaderData, 0, sizeof(Header));
   ArangeDescriptors.clear();
 }
 
-bool
-DWARFDebugArangeSet::extract(DataExtractor data, uint64_t *offset_ptr) {
-  if (data.isValidOffset(*offset_ptr)) {
-    ArangeDescriptors.clear();
-    Offset = *offset_ptr;
+Error DWARFDebugArangeSet::extract(DataExtractor data, uint64_t *offset_ptr) {
+  assert(data.isValidOffset(*offset_ptr));
 
-    // 7.20 Address Range Table
-    //
-    // Each set of entries in the table of address ranges contained in
-    // the .debug_aranges section begins with a header consisting of: a
-    // 4-byte length containing the length of the set of entries for this
-    // compilation unit, not including the length field itself; a 2-byte
-    // version identifier containing the value 2 for DWARF Version 2; a
-    // 4-byte offset into the.debug_infosection; a 1-byte unsigned integer
-    // containing the size in bytes of an address (or the offset portion of
-    // an address for segmented addressing) on the target system; and a
-    // 1-byte unsigned integer containing the size in bytes of a segment
-    // descriptor on the target system. This header is followed by a series
-    // of tuples. Each tuple consists of an address and a length, each in
-    // the size appropriate for an address on the target architecture.
-    HeaderData.Length = data.getU32(offset_ptr);
-    HeaderData.Version = data.getU16(offset_ptr);
-    HeaderData.CuOffset = data.getU32(offset_ptr);
-    HeaderData.AddrSize = data.getU8(offset_ptr);
-    HeaderData.SegSize = data.getU8(offset_ptr);
 
-    // Perform basic validation of the header fields.
-    if (!data.isValidOffsetForDataOfSize(Offset, HeaderData.Length) ||
-        (HeaderData.AddrSize != 4 && HeaderData.AddrSize != 8)) {
-      clear();
-      return false;
-    }
+  if (!data.isValidOffsetForDataOfSize(Offset, HeaderData.Length))
+    return createStringError(errc::invalid_argument,
+                             "Invalid arange header length");
+  if (HeaderData.AddrSize != 4 && HeaderData.AddrSize != 8)
+    return createStringError(errc::invalid_argument,
+                             "Invalid arange header address size");
 
-    // The first tuple following the header in each set begins at an offset
-    // that is a multiple of the size of a single tuple (that is, twice the
-    // size of an address). The header is padded, if necessary, to the
-    // appropriate boundary.
-    const uint32_t header_size = *offset_ptr - Offset;
-    const uint32_t tuple_size = HeaderData.AddrSize * 2;
-    uint32_t first_tuple_offset = 0;
-    while (first_tuple_offset < header_size)
-      first_tuple_offset += tuple_size;
 
-    *offset_ptr = Offset + first_tuple_offset;
 
-    Descriptor arangeDescriptor;
 
-    static_assert(sizeof(arangeDescriptor.Address) ==
-                      sizeof(arangeDescriptor.Length),
-                  "Different datatypes for addresses and sizes!");
-    assert(sizeof(arangeDescriptor.Address) >= HeaderData.AddrSize);
 
-    while (data.isValidOffset(*offset_ptr)) {
-      arangeDescriptor.Address = data.getUnsigned(offset_ptr, HeaderData.AddrSize);
-      arangeDescriptor.Length = data.getUnsigned(offset_ptr, HeaderData.AddrSize);
 
-      // Each set of tuples is terminated by a 0 for the address and 0
-      // for the length.
-      if (arangeDescriptor.Address || arangeDescriptor.Length)
-        ArangeDescriptors.push_back(arangeDescriptor);
-      else
-        break; // We are done if we get a zero address and length
-    }
+    if (!arangeDescriptor.Address && !arangeDescriptor.Length)
+      return ErrorSuccess();
 
-    return !ArangeDescriptors.empty();
-  }
-  return false;
+  return createStringError(errc::invalid_argument,
+                           "arange descriptors not terminated by null entry");
 }
 
 void DWARFDebugArangeSet::dump(raw_ostream &OS) const {
   OS << format("Address Range Header: length = 0x%8.8x, version = 0x%4.4x, ",
                HeaderData.Length, HeaderData.Version)
      << format("cu_offset = 0x%8.8x, addr_size = 0x%2.2x, seg_size = 0x%2.2x\n",
                HeaderData.CuOffset, HeaderData.AddrSize, HeaderData.SegSize);
 
   for (const auto &Desc : ArangeDescriptors) {
     Desc.dump(OS, HeaderData.AddrSize);
     OS << '\n';
   }
 }

llvm/lib/DebugInfo/DWARF/DWARFDebugAranges.cpp

 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <set>
 #include <vector>
 
 using namespace llvm;
 
 void DWARFDebugAranges::extract(DataExtractor DebugArangesData) {
   if (!DebugArangesData.isValidOffset(0))
     return;
   uint64_t Offset = 0;
   DWARFDebugArangeSet Set;
 
-  while (Set.extract(DebugArangesData, &Offset)) {
+  while (DebugArangesData.isValidOffset(Offset) &&
+         !errorToBool(Set.extract(DebugArangesData, &Offset))) {
     uint64_t CUOffset = Set.getCompileUnitDIEOffset();
     for (const auto &Desc : Set.descriptors()) {
       uint64_t LowPC = Desc.Address;
       uint64_t HighPC = Desc.getEndAddress();
       appendRange(CUOffset, LowPC, HighPC);
     }
     ParsedCUOffsets.insert(CUOffset);
   }
 }

llvm/test/DebugInfo/X86/dwarfdump-debug-aranges-error.s

+# RUN: llvm-mc -triple x86_64-unknown-linux %s -filetype=obj -o - | \
+# RUN:  llvm-dwarfdump -debug-aranges - 2>&1 | \
+# RUN:  FileCheck %s
+
+    .section .debug_aranges,"",@progbits
+    .long   .AREnd - .ARVersion # Length
+.ARVersion:
+    .short  2                   # Version
+    .long   0                   # Debug Info Offset
+    .byte   5                   # Address Size (invalid)
+    .byte   0                   # Segment Selector Size
+    .balign 0+4+4               # The first tuple alignment
+.ARTable:
+    .long   0, 1                # Address and size
+    .long   0, 0                # Termination tuple
+.AREnd:
+
+# CHECK: Invalid arange header address size

llvm/tools/obj2yaml/dwarf2yaml.cpp

 }
 
 void dumpDebugARanges(DWARFContext &DCtx, DWARFYAML::Data &Y) {
   DataExtractor ArangesData(DCtx.getDWARFObj().getArangesSection(),
                             DCtx.isLittleEndian(), 0);
   uint64_t Offset = 0;
   DWARFDebugArangeSet Set;
 
-  while (Set.extract(ArangesData, &Offset)) {
+  while (ArangesData.isValidOffset(Offset) &&
+         !errorToBool(Set.extract(ArangesData, &Offset))) {
     DWARFYAML::ARange Range;
     Range.Length.setLength(Set.getHeader().Length);
     Range.Version = Set.getHeader().Version;
     Range.CuOffset = Set.getHeader().CuOffset;
     Range.AddrSize = Set.getHeader().AddrSize;
     Range.SegSize = Set.getHeader().SegSize;
     for (auto Descriptor : Set.descriptors()) {
       DWARFYAML::ARangeDescriptor Desc;
       Desc.Address = Descriptor.Address;