[DebugInfo] Replace unreachable with ReportError in debug frame parser.

[JDevlieghere]

Summary

The parser should not rely on assertions or unreachables for invalid
input. This patch replaces calls to llvm::unreachable with calls to
ReportError in the debug frame parser.

This fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=3519
Test case is taken from there.

[rnk]

@MatzeB has proposed making report_fatal_error abort, which libfuzzer will re-file as a bug. Converting unreachable to report_fatal_error just kicks the actual error handling down the road.

[aprantl]

Since we are working towards the goal of making libDebugInfo good enough for LLDB to use, I don't think using report_fatal_error is acceptable on invalid inputs. Could this be made so the parsing returns an optional or expected instead?

[JDevlieghere]

Thanks guys, I should've given this more thought!

In D38462#885890, @rnk wrote:

@MatzeB has proposed making report_fatal_error abort, which libfuzzer will re-file as a bug. Converting unreachable to report_fatal_error just kicks the actual error handling down the road.

That makes sense.

In D38462#885897, @aprantl wrote:

Since we are working towards the goal of making libDebugInfo good enough for LLDB to use, I don't think using report_fatal_error is acceptable on invalid inputs. Could this be made so the parsing returns an optional or expected instead?

I think this is indeed the desired solution.

lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

 #include <cinttypes>
 #include <cstdint>
 #include <string>
 #include <vector>
 
 using namespace llvm;
 using namespace dwarf;
 
+// This is a workaround for old compilers which do not allow
+// noreturn attribute usage in lambdas. Once the support for those
+// compilers are phased out, we can remove this and return back to
+// a ReportError lambda: [StartOffset](const char *ErrorMsg).
+static void LLVM_ATTRIBUTE_NORETURN ReportError(uint32_t StartOffset,
+                                                const char *ErrorMsg) {
+  std::string Str;
+  raw_string_ostream OS(Str);
+  OS << format(ErrorMsg, StartOffset);
+  OS.flush();
+  report_fatal_error(Str);
+}
+
 /// \brief Abstract frame entry defining the common interface concrete
 /// entries implement.
 class llvm::FrameEntry {
 public:
   enum FrameKind {FK_CIE, FK_FDE};
 
   FrameEntry(FrameKind K, uint64_t Offset, uint64_t Length)
       : Kind(K), Offset(Offset), Length(Length) {}
while (*Offset < EndOffset) {
     // Some instructions have a primary opcode encoded in the top bits.
     uint8_t Primary = Opcode & DWARF_CFI_PRIMARY_OPCODE_MASK;
 
     if (Primary) {
       // If it's a primary opcode, the first operand is encoded in the bottom
       // bits of the opcode itself.
       uint64_t Op1 = Opcode & DWARF_CFI_PRIMARY_OPERAND_MASK;
       switch (Primary) {
-        default: llvm_unreachable("Impossible primary CFI opcode");
-        case DW_CFA_advance_loc:
-        case DW_CFA_restore:
-          addInstruction(Primary, Op1);
-          break;
-        case DW_CFA_offset:
-          addInstruction(Primary, Op1, Data.getULEB128(Offset));
-          break;
+      default:
+        ReportError(Primary, "Impossible primary CFI opcode: %d");
       }
     } else {
       // Extended opcode - its value is Opcode itself.
       switch (Opcode) {
-        default: llvm_unreachable("Invalid extended CFI opcode");
-        case DW_CFA_nop:
-        case DW_CFA_remember_state:
-        case DW_CFA_restore_state:
-        case DW_CFA_GNU_window_save:
-          // No operands
-          addInstruction(Opcode);
-          break;
-        case DW_CFA_set_loc:
-          // Operands: Address
-          addInstruction(Opcode, Data.getAddress(Offset));
-          break;
-        case DW_CFA_advance_loc1:
-          // Operands: 1-byte delta
-          addInstruction(Opcode, Data.getU8(Offset));
-          break;
-        case DW_CFA_advance_loc2:
-          // Operands: 2-byte delta
-          addInstruction(Opcode, Data.getU16(Offset));
-          break;
-        case DW_CFA_advance_loc4:
-          // Operands: 4-byte delta
-          addInstruction(Opcode, Data.getU32(Offset));
-          break;
-        case DW_CFA_restore_extended:
-        case DW_CFA_undefined:
-        case DW_CFA_same_value:
-        case DW_CFA_def_cfa_register:
-        case DW_CFA_def_cfa_offset:
-          // Operands: ULEB128
-          addInstruction(Opcode, Data.getULEB128(Offset));
-          break;
-        case DW_CFA_def_cfa_offset_sf:
-          // Operands: SLEB128
-          addInstruction(Opcode, Data.getSLEB128(Offset));
-          break;
-        case DW_CFA_offset_extended:
-        case DW_CFA_register:
-        case DW_CFA_def_cfa:
-        case DW_CFA_val_offset: {
-          // Operands: ULEB128, ULEB128
-          // Note: We can not embed getULEB128 directly into function
-          // argument list. getULEB128 changes Offset and order of evaluation
-          // for arguments is unspecified.
-          auto op1 = Data.getULEB128(Offset);
-          auto op2 = Data.getULEB128(Offset);
-          addInstruction(Opcode, op1, op2);
-          break;
+      default:
+        ReportError(Opcode, "Invalid extended CFI opcode: %d");
         }
         case DW_CFA_offset_extended_sf:
         case DW_CFA_def_cfa_sf:
         case DW_CFA_val_offset_sf: {
           // Operands: ULEB128, SLEB128
           // Note: see comment for the previous case
           auto op1 = Data.getULEB128(Offset);
           auto op2 = (uint64_t)Data.getSLEB128(Offset);
for (int i = 0; i < Length; ++i) {
     uint8_t c = Data.getU8(&Offset);
     errs().write_hex(c); errs() << " ";
   }
   errs() << "\n";
 }
 
 static unsigned getSizeForEncoding(const DataExtractor &Data,
                                    unsigned symbolEncoding) {
-  unsigned format = symbolEncoding & 0x0f;
-  switch (format) {
-    default: llvm_unreachable("Unknown Encoding");
-    case DW_EH_PE_absptr:
-    case DW_EH_PE_signed:
-      return Data.getAddressSize();
-    case DW_EH_PE_udata2:
-    case DW_EH_PE_sdata2:
-      return 2;
-    case DW_EH_PE_udata4:
-    case DW_EH_PE_sdata4:
-      return 4;
-    case DW_EH_PE_udata8:
-    case DW_EH_PE_sdata8:
-      return 8;
+  unsigned Format = symbolEncoding & 0x0f;
+  switch (Format) {
+  default:
+    ReportError(Format, "Unknown uncoding: %d");
   }
 }
 
 static uint64_t readPointer(const DataExtractor &Data, uint32_t &Offset,
                             unsigned Encoding) {
-  switch (getSizeForEncoding(Data, Encoding)) {
-    case 2:
-      return Data.getU16(&Offset);
-    case 4:
-      return Data.getU32(&Offset);
-    case 8:
-      return Data.getU64(&Offset);
-    default:
-      llvm_unreachable("Illegal data size");
-  }
+  unsigned Size = getSizeForEncoding(Data, Encoding);
+  switch (Size) {
+    ReportError(Size, "Illegal data size: %d");
   }
-
-// This is a workaround for old compilers which do not allow
-// noreturn attribute usage in lambdas. Once the support for those
-// compilers are phased out, we can remove this and return back to
-// a ReportError lambda: [StartOffset](const char *ErrorMsg).
-static void LLVM_ATTRIBUTE_NORETURN ReportError(uint32_t StartOffset,
-                                                const char *ErrorMsg) {
-  std::string Str;
-  raw_string_ostream OS(Str);
-  OS << format(ErrorMsg, StartOffset);
-  OS.flush();
-  report_fatal_error(Str);
 }
 
 void DWARFDebugFrame::parse(DataExtractor Data) {
   uint32_t Offset = 0;
   DenseMap<uint32_t, CIE *> CIEs;
 
   while (Data.isValidOffset(Offset)) {
     uint32_t StartOffset = Offset;

test/DebugInfo/dwarfdump-invalid-extended-cfi-opcode.test

+RUN: not llvm-dwarfdump -debug-frame %p/Inputs/dwarfdump-invalid-extended-cfi-opcode.wasm 2>&1 | FileCheck %s
+
+CHECK: LLVM ERROR: Invalid extended CFI opcode: 41