[llvm-dwarfdump] - Teach verifier to report broken DWARF expressions.

[grimar]

Summary

Patch improves next things:

1. Fixes assert/crash in getOpDesc when giving it a invalid expression op code.
2. DWARFExpression::print() called DWARFExpression::Operation::getEndOffset() which returned and used uninitialized field EndOffset. Patch fixes that.
3. Teaches verifier to verify DW_AT_location and error out on broken expressions.

[aprantl]

Thanks!

lib/DebugInfo/DWARF/DWARFVerifier.cpp

384 ↗

(On Diff #120268)

perhaps also add the << ":\n" ?

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

12 ↗

(On Diff #120268)

where does the . after error come from? it looks out of place. Perhaps <decoding error> would be better?

[JDevlieghere]

lib/DebugInfo/DWARF/DWARFVerifier.cpp

405 ↗

(On Diff #120268)

Can we use ReportError here too?

[JDevlieghere]

lib/DebugInfo/DWARF/DWARFVerifier.cpp

424 ↗

(On Diff #120268)

I don't think we can avoid the cast, but let's use reinterpret_cast here.

[grimar]

• Addressed review comments.
• Minor cosmetic change.

lib/DebugInfo/DWARF/DWARFVerifier.cpp

384 ↗

(On Diff #120268)

Added \n, can't add :\n because have to handle reporting for DW_AT_stmt_list below.

405 ↗

(On Diff #120268)

Done.

424 ↗

(On Diff #120268)

Done.

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

12 ↗

(On Diff #120268)

. comes from https://github.com/llvm-mirror/llvm/blob/master/lib/DebugInfo/DWARF/DWARFExpression.cpp#L224.

It does not seem this message was ever tested before, so I removed the dot.

<decoding error> looks better for me, but I would not change () to <>
in this patch as it comes from some different place and probably should break a lot of tests,
so looks should be done as separate change (if should).

[JDevlieghere]

Thanks, George! LGTM

[aprantl]

Thanks!

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

12 ↗

(On Diff #120268)

This is obviously not super important, but I thought of
DW_AT_location (<decoding error>)
The <> is meant to "escape" the error message and make it look different from an actual value.

[grimar]

Adrian, Jonas, thank you for review !

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

12 ↗

(On Diff #120268)

Ah, there is no problems to do that in this patch then I believe,
I'll change to (<decoding error>) before commit.

llvm/trunk/lib/DebugInfo/DWARF/DWARFExpression.cpp

static DescVector getDescriptions() {
   Descriptions[DW_OP_GNU_addr_index] = Desc(Op::Dwarf4, Op::SizeLEB);
   Descriptions[DW_OP_GNU_const_index] = Desc(Op::Dwarf4, Op::SizeLEB);
   return Descriptions;
 }
 
 static DWARFExpression::Operation::Description getOpDesc(unsigned OpCode) {
   // FIXME: Make this constexpr once all compilers are smart enough to do it.
   static DescVector Descriptions = getDescriptions();
-  assert(OpCode < Descriptions.size());
+  // Handle possible corrupted or unsupported operation.
+  if (OpCode >= Descriptions.size())
+    return {};
   return Descriptions[OpCode];
 }
 
 static uint8_t getRefAddrSize(uint8_t AddrSize, uint16_t Version) {
   return (Version == 2) ? AddrSize : 4;
 }
 
 bool DWARFExpression::Operation::extract(DataExtractor Data, uint16_t Version,
                                          uint8_t AddressSize, uint32_t Offset) {
   Opcode = Data.getU8(&Offset);
 
   Desc = getOpDesc(Opcode);
-  if (Desc.Version == Operation::DwarfNA)
+  if (Desc.Version == Operation::DwarfNA) {
+    EndOffset = Offset;
     return false;
+  }
 
   for (unsigned Operand = 0; Operand < 2; ++Operand) {
     unsigned Size = Desc.Op[Operand];
     unsigned Signed = Size & Operation::SignBit;
 
     if (Size == Operation::SizeNA)
       break;
 
static bool prettyPrintRegisterOp(raw_ostream &OS, uint8_t Opcode,
   return false;
 }
 
 bool DWARFExpression::Operation::print(raw_ostream &OS,
                                        const DWARFExpression *Expr,
                                        const MCRegisterInfo *RegInfo,
                                        bool isEH) {
   if (Error) {
-    OS << "decoding error.";
+    OS << "<decoding error>";
     return false;
   }
 
   StringRef Name = OperationEncodingString(Opcode);
   assert(!Name.empty() && "DW_OP has no name!");
   OS << Name;
 
   if ((Opcode >= DW_OP_breg0 && Opcode <= DW_OP_breg31) ||

llvm/trunk/lib/DebugInfo/DWARF/DWARFVerifier.cpp

 //===- DWARFVerifier.cpp --------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "SyntaxHighlighting.h"
 #include "llvm/DebugInfo/DWARF/DWARFVerifier.h"
 #include "llvm/DebugInfo/DWARF/DWARFCompileUnit.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugLine.h"
 #include "llvm/DebugInfo/DWARF/DWARFDie.h"
+#include "llvm/DebugInfo/DWARF/DWARFExpression.h"
 #include "llvm/DebugInfo/DWARF/DWARFFormValue.h"
 #include "llvm/DebugInfo/DWARF/DWARFSection.h"
 #include "llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/raw_ostream.h"
 #include <map>
 #include <set>
 #include <vector>
 
 using namespace llvm;
 using namespace dwarf;
 using namespace object;
unsigned DWARFVerifier::verifyDieRanges(const DWARFDie &Die,
   for (DWARFDie Child : Die)
     NumErrors += verifyDieRanges(Child, RI);
 
   return NumErrors;
 }
 
 unsigned DWARFVerifier::verifyDebugInfoAttribute(const DWARFDie &Die,
                                                  DWARFAttribute &AttrValue) {
-  const DWARFObject &DObj = DCtx.getDWARFObj();
   unsigned NumErrors = 0;
+  auto ReportError = [&](const Twine &TitleMsg) {
+    ++NumErrors;
+    error() << TitleMsg << '\n';
+    Die.dump(OS, 0, DumpOpts);
+    OS << "\n";
+  };
+
+  const DWARFObject &DObj = DCtx.getDWARFObj();
   const auto Attr = AttrValue.Attr;
   switch (Attr) {
   case DW_AT_ranges:
     // Make sure the offset in the DW_AT_ranges attribute is valid.
     if (auto SectionOffset = AttrValue.Value.getAsSectionOffset()) {
-      if (*SectionOffset >= DObj.getRangeSection().Data.size()) {
-        ++NumErrors;
-        error() << "DW_AT_ranges offset is beyond .debug_ranges "
-                   "bounds:\n";
-        Die.dump(OS, 0, DumpOpts);
-        OS << "\n";
-      }
-    } else {
-      ++NumErrors;
-      error() << "DIE has invalid DW_AT_ranges encoding:\n";
-      Die.dump(OS, 0, DumpOpts);
-      OS << "\n";
+      if (*SectionOffset >= DObj.getRangeSection().Data.size())
+        ReportError("DW_AT_ranges offset is beyond .debug_ranges bounds:");
+      break;
     }
+    ReportError("DIE has invalid DW_AT_ranges encoding:");
     break;
   case DW_AT_stmt_list:
     // Make sure the offset in the DW_AT_stmt_list attribute is valid.
     if (auto SectionOffset = AttrValue.Value.getAsSectionOffset()) {
-      if (*SectionOffset >= DObj.getLineSection().Data.size()) {
-        ++NumErrors;
-        error() << "DW_AT_stmt_list offset is beyond .debug_line "
-                   "bounds: "
-                << format("0x%08" PRIx64, *SectionOffset) << "\n";
-        Die.dump(OS, 0, DumpOpts);
-        OS << "\n";
-      }
-    } else {
-      ++NumErrors;
-      error() << "DIE has invalid DW_AT_stmt_list encoding:\n";
-      Die.dump(OS, 0, DumpOpts);
-      OS << "\n";
+      if (*SectionOffset >= DObj.getLineSection().Data.size())
+        ReportError("DW_AT_stmt_list offset is beyond .debug_line bounds: " +
+                    llvm::formatv("{0:x16}", *SectionOffset));
+      break;
+    ReportError("DIE has invalid DW_AT_stmt_list encoding:");
+    break;
+  case DW_AT_location: {
+    Optional<ArrayRef<uint8_t>> Expr = AttrValue.Value.getAsBlock();
+    if (!Expr) {
+      ReportError("DIE has invalid DW_AT_location encoding:");
+      break;
     }
+
+    DWARFUnit *U = Die.getDwarfUnit();
+    DataExtractor Data(
+        StringRef(reinterpret_cast<const char *>(Expr->data()), Expr->size()),
+        DCtx.isLittleEndian(), 0);
+    DWARFExpression Expression(Data, U->getVersion(), U->getAddressByteSize());
+    bool Error = llvm::any_of(Expression, [](DWARFExpression::Operation &Op) {
+      return Op.isError();
+    });
+    if (Error)
+      ReportError("DIE contains invalid DWARF expression:");
     break;
+  }
 
   default:
     break;
   }
   return NumErrors;
 }
 
 unsigned DWARFVerifier::verifyDebugInfoForm(const DWARFDie &Die,

llvm/trunk/test/tools/llvm-dwarfdump/X86/verify_broken_exprloc.s

+# RUN: llvm-mc %s -filetype obj -triple i686-pc-linux -o %t
+
+## Check we don't crash when parsing invalid expression opcode.
+# RUN: llvm-dwarfdump %t | FileCheck %s
+# CHECK:      DW_TAG_GNU_call_site_parameter
+# CHECK-NEXT:  DW_AT_location  (<decoding error>)
+
+## Check verifier reports an error.
+# RUN: not llvm-dwarfdump -verify %t 2>&1 | FileCheck %s --check-prefix=VERIFY
+# VERIFY:      DIE contains invalid DWARF expression:
+# VERIFY:      DW_TAG_GNU_call_site_parameter
+# VERIFY-NEXT:   DW_AT_location    (<decoding error>)
+
+.section  .debug_info,"",@progbits
+  .long  0x12
+  .value  0x4
+  .long  0
+  .byte  0x4
+
+  .uleb128 0x1 # DW_TAG_compile_unit [1]
+  .long  0
+  .byte  0x0
+
+  .uleb128 0x2  # DW_TAG_GNU_call_site_parameter [2]
+  .uleb128 0x1  # Expression size.
+  .byte  0xff   # Broken expression.
+
+  .byte  0      # End mark.
+  .byte  0      # End mark.
+
+.section .debug_abbrev,"",@progbits
+  .uleb128 0x1    # ID [1]
+  .uleb128 0x11   # DW_TAG_compile_unit, DW_CHILDREN_yes
+  .byte  0x1
+  .uleb128 0x25   # DW_AT_producer, DW_FORM_strp
+  .uleb128 0xe
+  .uleb128 0x13   # DW_AT_language, DW_FORM_data1
+  .uleb128 0xb
+  .byte  0
+  .byte  0
+
+  .uleb128 0x2    # ID [2]
+  .uleb128 0x410a # DW_TAG_GNU_call_site_parameter, DW_CHILDREN_no
+  .byte  0
+  .uleb128 0x2    # DW_AT_location, DW_FORM_exprloc
+  .uleb128 0x18
+  .byte  0
+  .byte  0
+  .byte  0
+
+.section .debug_str,"MS",@progbits,1
+.string "test"