DebugInfo: Sink string form validation down from verifier to form parsing

dwblaikie · dwblaikie · commit 92f2d02b4a31 · 2021-12-14T15:41:53.000-08:00
Avoid duplicating the string decoding - improve the error messages down
in form parsing (&amp; produce an Expected&lt;const char*&gt; instead of
Optional&lt;const char*&gt; to communicate the extra error details)
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h
@@ -112,7 +112,7 @@ class DWARFFormValue {
   Optional<UnitOffset> getAsRelativeReference() const;
   Optional<uint64_t> getAsUnsignedConstant() const;
   Optional<int64_t> getAsSignedConstant() const;
-  Optional<const char *> getAsCString() const;
+  Expected<const char *> getAsCString() const;
   Optional<uint64_t> getAsAddress() const;
   Optional<object::SectionedAddress> getAsSectionedAddress() const;
   Optional<uint64_t> getAsSectionOffset() const;
@@ -173,9 +173,14 @@ namespace dwarf {
 /// \returns an optional value that contains a value if the form value
 /// was valid and was a string.
 inline Optional<const char *> toString(const Optional<DWARFFormValue> &V) {
-  if (V)
-    return V->getAsCString();
-  return None;
+  if (!V)
+    return None;
+  Expected<const char*> E = V->getAsCString();
+  if (!E) {
+    consumeError(E.takeError());
+    return None;
+  }
+  return *E;
 }
 
 /// Take an optional DWARFFormValue and try to extract a string value from it.
@@ -185,10 +190,16 @@ inline Optional<const char *> toString(const Optional<DWARFFormValue> &V) {
 /// was valid and was a string.
 inline StringRef toStringRef(const Optional<DWARFFormValue> &V,
                              StringRef Default = {}) {
-  if (V)
-    if (auto S = V->getAsCString())
-      return *S;
-  return Default;
+  if (!V)
+    return Default;
+  auto S = V->getAsCString();
+  if (!S) {
+    consumeError(S.takeError());
+    return Default;
+  }
+  if (!*S)
+    return Default;
+  return *S;
 }
 
 /// Take an optional DWARFFormValue and extract a string value from it.
diff --git a/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h b/llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
@@ -331,7 +331,7 @@ class DWARFUnit {
 
   Optional<object::SectionedAddress>
   getAddrOffsetSectionItem(uint32_t Index) const;
-  Optional<uint64_t> getStringOffsetSectionItem(uint32_t Index) const;
+  Expected<uint64_t> getStringOffsetSectionItem(uint32_t Index) const;
 
   DWARFDataExtractor getDebugInfoExtractor() const;
 
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp b/llvm/lib/DebugInfo/DWARF/DWARFDebugMacro.cpp
@@ -194,13 +194,11 @@ Error DWARFDebugMacro::parseImpl(
       if (MacroContributionOffset == MacroToUnits.end())
         return createStringError(errc::invalid_argument,
                                  "Macro contribution of the unit not found");
-      Optional<uint64_t> StrOffset =
+      Expected<uint64_t> StrOffset =
           MacroContributionOffset->second->getStringOffsetSectionItem(
               Data.getULEB128(&Offset));
       if (!StrOffset)
-        return createStringError(
-            errc::invalid_argument,
-            "String offsets contribution of the unit not found");
+        return StrOffset.takeError();
       E.MacroStr =
           MacroContributionOffset->second->getStringExtractor().getCStr(
               &*StrOffset);
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp b/llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
@@ -613,50 +613,53 @@ void DWARFFormValue::dump(raw_ostream &OS, DIDumpOptions DumpOpts) const {
 }
 
 void DWARFFormValue::dumpString(raw_ostream &OS) const {
-  Optional<const char *> DbgStr = getAsCString();
-  if (DbgStr.hasValue()) {
+  if (auto DbgStr = dwarf::toString(*this)) {
     auto COS = WithColor(OS, HighlightColor::String);
     COS.get() << '"';
-    COS.get().write_escaped(DbgStr.getValue());
+    COS.get().write_escaped(*DbgStr);
     COS.get() << '"';
   }
 }
 
-Optional<const char *> DWARFFormValue::getAsCString() const {
+Expected<const char *> DWARFFormValue::getAsCString() const {
   if (!isFormClass(FC_String))
-    return None;
+    return make_error<StringError>("Invalid form for string attribute",
+                                   inconvertibleErrorCode());
   if (Form == DW_FORM_string)
     return Value.cstr;
   // FIXME: Add support for DW_FORM_GNU_strp_alt
   if (Form == DW_FORM_GNU_strp_alt || C == nullptr)
-    return None;
+    return make_error<StringError>("Unsupported form for string attribute",
+                                   inconvertibleErrorCode());
   uint64_t Offset = Value.uval;
-  if (Form == DW_FORM_line_strp) {
-    // .debug_line_str is tracked in the Context.
-    if (const char *Str = C->getLineStringExtractor().getCStr(&Offset))
-      return Str;
-    return None;
-  }
+  Optional<uint32_t> Index;
   if (Form == DW_FORM_GNU_str_index || Form == DW_FORM_strx ||
       Form == DW_FORM_strx1 || Form == DW_FORM_strx2 || Form == DW_FORM_strx3 ||
       Form == DW_FORM_strx4) {
     if (!U)
-      return None;
-    Optional<uint64_t> StrOffset = U->getStringOffsetSectionItem(Offset);
+      return make_error<StringError>("API limitation - string extraction not "
+                                     "available without a DWARFUnit",
+                                     inconvertibleErrorCode());
+    Expected<uint64_t> StrOffset = U->getStringOffsetSectionItem(Offset);
+    Index = Offset;
     if (!StrOffset)
-      return None;
+      return StrOffset.takeError();
     Offset = *StrOffset;
   }
   // Prefer the Unit's string extractor, because for .dwo it will point to
   // .debug_str.dwo, while the Context's extractor always uses .debug_str.
-  if (U) {
-    if (const char *Str = U->getStringExtractor().getCStr(&Offset))
-      return Str;
-    return None;
-  }
-  if (const char *Str = C->getStringExtractor().getCStr(&Offset))
+  DataExtractor StrData = Form == DW_FORM_line_strp
+                              ? C->getLineStringExtractor()
+                          : U ? U->getStringExtractor()
+                              : C->getStringExtractor();
+  if (const char *Str = StrData.getCStr(&Offset))
     return Str;
-  return None;
+  std::string Msg = FormEncodingString(Form).str();
+  if (Index)
+    Msg += (" uses index " + Twine(*Index) + ", but the referenced string").str();
+  Msg += (" offset " + Twine(Offset) + " is beyond .debug_str bounds").str();
+  return make_error<StringError>(Msg,
+      inconvertibleErrorCode());
 }
 
 Optional<uint64_t> DWARFFormValue::getAsAddress() const {
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp b/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
@@ -214,13 +214,17 @@ DWARFUnit::getAddrOffsetSectionItem(uint32_t Index) const {
   return {{Address, Section}};
 }
 
-Optional<uint64_t> DWARFUnit::getStringOffsetSectionItem(uint32_t Index) const {
+Expected<uint64_t> DWARFUnit::getStringOffsetSectionItem(uint32_t Index) const {
   if (!StringOffsetsTableContribution)
-    return None;
+    return make_error<StringError>(
+        "DW_FORM_strx used without a valid string offsets table",
+        inconvertibleErrorCode());
   unsigned ItemSize = getDwarfStringOffsetsByteSize();
   uint64_t Offset = getStringOffsetsBase() + Index * ItemSize;
   if (StringOffsetSection.Data.size() < Offset + ItemSize)
-    return None;
+    return make_error<StringError>("DW_FORM_strx uses index " + Twine(Index) +
+                                       ", which is too large",
+                                   inconvertibleErrorCode());
   DWARFDataExtractor DA(Context.getDWARFObj(), StringOffsetSection,
                         isLittleEndian, 0);
   return DA.getRelocatedValue(ItemSize, &Offset);
diff --git a/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp b/llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
@@ -616,7 +616,6 @@ unsigned DWARFVerifier::verifyDebugInfoForm(const DWARFDie &Die,
                                             DWARFAttribute &AttrValue,
                                             ReferenceMap &LocalReferences,
                                             ReferenceMap &CrossUnitReferences) {
-  const DWARFObject &DObj = DCtx.getDWARFObj();
   auto DieCU = Die.getDwarfUnit();
   unsigned NumErrors = 0;
   const auto Form = AttrValue.Value.getForm();
@@ -667,51 +666,15 @@ unsigned DWARFVerifier::verifyDebugInfoForm(const DWARFDie &Die,
     }
     break;
   }
-  case DW_FORM_strp: {
-    auto SecOffset = AttrValue.Value.getAsSectionOffset();
-    assert(SecOffset); // DW_FORM_strp is a section offset.
-    if (SecOffset && *SecOffset >= DObj.getStrSection().size()) {
-      ++NumErrors;
-      error() << "DW_FORM_strp offset beyond .debug_str bounds:\n";
-      dump(Die) << '\n';
-    }
-    break;
-  }
+  case DW_FORM_strp:
   case DW_FORM_strx:
   case DW_FORM_strx1:
   case DW_FORM_strx2:
   case DW_FORM_strx3:
   case DW_FORM_strx4: {
-    auto Index = AttrValue.Value.getRawUValue();
-    auto DieCU = Die.getDwarfUnit();
-    // Check that we have a valid DWARF v5 string offsets table.
-    if (!DieCU->getStringOffsetsTableContribution()) {
-      ++NumErrors;
-      error() << FormEncodingString(Form)
-              << " used without a valid string offsets table:\n";
-      dump(Die) << '\n';
-      break;
-    }
-    // Check that the index is within the bounds of the section.
-    unsigned ItemSize = DieCU->getDwarfStringOffsetsByteSize();
-    // Use a 64-bit type to calculate the offset to guard against overflow.
-    uint64_t Offset =
-        (uint64_t)DieCU->getStringOffsetsBase() + Index * ItemSize;
-    if (DObj.getStrOffsetsSection().Data.size() < Offset + ItemSize) {
-      ++NumErrors;
-      error() << FormEncodingString(Form) << " uses index "
-              << format("%" PRIu64, Index) << ", which is too large:\n";
-      dump(Die) << '\n';
-      break;
-    }
-    // Check that the string offset is valid.
-    uint64_t StringOffset = *DieCU->getStringOffsetSectionItem(Index);
-    if (StringOffset >= DObj.getStrSection().size()) {
+    if (Error E = AttrValue.Value.getAsCString().takeError()) {
       ++NumErrors;
-      error() << FormEncodingString(Form) << " uses index "
-              << format("%" PRIu64, Index)
-              << ", but the referenced string"
-                 " offset is beyond .debug_str bounds:\n";
+      error() << toString(std::move(E)) << ":\n";
       dump(Die) << '\n';
     }
     break;
diff --git a/llvm/test/DebugInfo/X86/debug-macro-empty-str-offset.s b/llvm/test/DebugInfo/X86/debug-macro-empty-str-offset.s
@@ -5,7 +5,7 @@
 # RUN: llvm-mc -triple x86_64-unknown-linux -filetype=obj %s -o -| \
 # RUN:   not llvm-dwarfdump -debug-macro - /dev/null 2>&1 | FileCheck %s
 
-# CHECK: error: String offsets contribution of the unit not found
+# CHECK: error: DW_FORM_strx used without a valid string offsets table
 
        .section        .debug_abbrev,"",@progbits
        .byte   1                       # Abbreviation Code
diff --git a/llvm/test/tools/llvm-dwarfdump/X86/verify_invalid_strp.yaml b/llvm/test/tools/llvm-dwarfdump/X86/verify_invalid_strp.yaml
@@ -2,7 +2,7 @@
 # RUN: not llvm-dwarfdump -debug-info -verify %t.o | FileCheck %s
 
 #      CHECK: Verifying non-dwo Units... 
-# CHECK-NEXT: error: DW_FORM_strp offset beyond .debug_str bounds:
+# CHECK-NEXT: error: DW_FORM_strp offset 4660 is beyond .debug_str bounds:
 
 --- !ELF
 FileHeader:
