[DebugInfo/DWARF] - Test invalid CFI opcodes properly and refine related `CFIProgram::parse` code.

[grimar]

Summary

There are following issues with CFIProgram::parse code:

1. Invalid CFI opcodes were never tested. And currently a test would fail

when the LLVM_ENABLE_ABI_BREAKING_CHECKS is enabled. It happens because
the DataExtractor::Cursor C remains unchecked when the
"Invalid extended CFI opcode" error is reported:

.eh_frame section at offset 0x1128 address 0x0:
Program aborted due to an unhandled Error:
Error value was Success. (Note: Success values must still be checked prior to being destroyed).

2. It is impossible to reach the "Invalid primary CFI opcode" error with the current code.

There are 3 possible primary opcode values and all of them are handled. Hence this error
should be replaced with llvm_unreachable.

3. Errors currently reported are upper-case.

This patch refines the code in the CFIProgram::parse method to fix all issues mentioned
and adds unit tests for all possible invalid extended CFI opcodes.

[grimar]

Ping.

[jhenderson]

LGTM, with nit.

llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp

124

parseCFI

llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp

 const uint8_t DWARF_CFI_PRIMARY_OPCODE_MASK = 0xc0;
 const uint8_t DWARF_CFI_PRIMARY_OPERAND_MASK = 0x3f;
 
 Error CFIProgram::parse(DWARFDataExtractor Data, uint64_t *Offset,
                         uint64_t EndOffset) {
   DataExtractor::Cursor C(*Offset);
   while (C && C.tell() < EndOffset) {
     uint8_t Opcode = Data.getRelocatedValue(C, 1);
-    // Some instructions have a primary opcode encoded in the top bits.
-    uint8_t Primary = Opcode & DWARF_CFI_PRIMARY_OPCODE_MASK;
+    if (!C)
+      break;
 
-    if (Primary) {
+    // Some instructions have a primary opcode encoded in the top bits.
+    if (uint8_t Primary = Opcode & DWARF_CFI_PRIMARY_OPCODE_MASK) {
       // 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:
-        return createStringError(errc::illegal_byte_sequence,
-                                 "Invalid primary CFI opcode 0x%" PRIx8,
-                                 Primary);
       case DW_CFA_advance_loc:
       case DW_CFA_restore:
         addInstruction(Primary, Op1);
         break;
       case DW_CFA_offset:
         addInstruction(Primary, Op1, Data.getULEB128(C));
         break;
+      default:
+        llvm_unreachable("invalid primary CFI opcode");
       }
-    } else {
-      // Extended opcode - its value is Opcode itself.
-      switch (Opcode) {
-      default:
-        return createStringError(errc::illegal_byte_sequence,
-                                 "Invalid extended CFI opcode 0x%" PRIx8,
-                                 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.getRelocatedAddress(C));
-        break;
-      case DW_CFA_advance_loc1:
-        // Operands: 1-byte delta
-        addInstruction(Opcode, Data.getRelocatedValue(C, 1));
-        break;
-      case DW_CFA_advance_loc2:
-        // Operands: 2-byte delta
-        addInstruction(Opcode, Data.getRelocatedValue(C, 2));
-        break;
-      case DW_CFA_advance_loc4:
-        // Operands: 4-byte delta
-        addInstruction(Opcode, Data.getRelocatedValue(C, 4));
-        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:
-      case DW_CFA_GNU_args_size:
-        // Operands: ULEB128
-        addInstruction(Opcode, Data.getULEB128(C));
-        break;
-      case DW_CFA_def_cfa_offset_sf:
-        // Operands: SLEB128
-        addInstruction(Opcode, Data.getSLEB128(C));
-        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.
-        uint64_t op1 = Data.getULEB128(C);
-        uint64_t op2 = Data.getULEB128(C);
-        addInstruction(Opcode, op1, op2);
-        break;
-      }
-      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
-        uint64_t op1 = Data.getULEB128(C);
-        uint64_t op2 = (uint64_t)Data.getSLEB128(C);
-        addInstruction(Opcode, op1, op2);
-        break;
-      }
-      case DW_CFA_def_cfa_expression: {
-        uint64_t ExprLength = Data.getULEB128(C);
-        addInstruction(Opcode, 0);
-        StringRef Expression = Data.getBytes(C, ExprLength);
+      continue;
+    }
+
+                               "invalid extended CFI opcode 0x%" PRIx8, Opcode);
 
-        DataExtractor Extractor(Expression, Data.isLittleEndian(),
-                                Data.getAddressSize());
-        // Note. We do not pass the DWARF format to DWARFExpression, because
-        // DW_OP_call_ref, the only operation which depends on the format, is
-        // prohibited in call frame instructions, see sec. 6.4.2 in DWARFv5.
-        Instructions.back().Expression =
-            DWARFExpression(Extractor, Data.getAddressSize());
-        break;
-      }
-      case DW_CFA_expression:
-      case DW_CFA_val_expression: {
-        uint64_t RegNum = Data.getULEB128(C);
-        addInstruction(Opcode, RegNum, 0);
 
-        uint64_t BlockLength = Data.getULEB128(C);
-        StringRef Expression = Data.getBytes(C, BlockLength);
-        DataExtractor Extractor(Expression, Data.isLittleEndian(),
-                                Data.getAddressSize());
-        // Note. We do not pass the DWARF format to DWARFExpression, because
-        // DW_OP_call_ref, the only operation which depends on the format, is
-        // prohibited in call frame instructions, see sec. 6.4.2 in DWARFv5.
-        Instructions.back().Expression =
-            DWARFExpression(Extractor, Data.getAddressSize());
-        break;
-      }
-      }
-    }
-  }
 
   *Offset = C.tell();
   return C.takeError();
 }
 
 namespace {
 
 

llvm/unittests/DebugInfo/DWARF/DWARFDebugFrameTest.cpp

 //===- llvm/unittest/DebugInfo/DWARFDebugFrameTest.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/ADT/DenseSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugFrame.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
dwarf::FDE TestFDE(/*IsDWARF64=*/true,
                      /*Cie=*/&TestCIE,
                      /*LSDAAddress=*/None,
                      /*Arch=*/Triple::x86_64);
   expectDumpResult(TestFDE, /*IsEH=*/true,
                    "1111abcdabcd 00002222abcdabcd 0033abcd FDE "
                    "cie=1111ab9a000c pc=4444abcdabcd...5555bcdebcde");
 }
 
-// Here we test how truncated Call Frame Instructions are parsed.
-TEST(DWARFDebugFrame, ParseTruncatedCFITest) {
-  auto ParseCFI = [](dwarf::CIE &C, ArrayRef<uint8_t> Instructions,
+static Error parseCFI(dwarf::CIE &C, ArrayRef<uint8_t> Instructions,
                       Optional<uint64_t> Size = None) {
-    DWARFDataExtractor Data(Instructions, /*IsLittleEndian=*/true,
-                            /*AddressSize=*/8);
-    uint64_t Offset = 0;
-    const uint64_t EndOffset = Size ? *Size : (uint64_t)Instructions.size();
-    return C.cfis().parse(Data, &Offset, EndOffset);
-  };
+}
+
+TEST(DWARFDebugFrame, InvalidCFIOpcodesTest) {
+  llvm::DenseSet<uint8_t> ValidExtendedOpcodes = {
+      dwarf::DW_CFA_nop,
+      dwarf::DW_CFA_advance_loc,
+      dwarf::DW_CFA_offset,
+      dwarf::DW_CFA_restore,
+      dwarf::DW_CFA_set_loc,
+      dwarf::DW_CFA_advance_loc1,
+      dwarf::DW_CFA_advance_loc2,
+      dwarf::DW_CFA_advance_loc4,
+      dwarf::DW_CFA_offset_extended,
+      dwarf::DW_CFA_restore_extended,
+      dwarf::DW_CFA_undefined,
+      dwarf::DW_CFA_same_value,
+      dwarf::DW_CFA_register,
+      dwarf::DW_CFA_remember_state,
+      dwarf::DW_CFA_restore_state,
+      dwarf::DW_CFA_def_cfa,
+      dwarf::DW_CFA_def_cfa_register,
+      dwarf::DW_CFA_def_cfa_offset,
+      dwarf::DW_CFA_def_cfa_expression,
+      dwarf::DW_CFA_expression,
+      dwarf::DW_CFA_offset_extended_sf,
+      dwarf::DW_CFA_def_cfa_sf,
+      dwarf::DW_CFA_def_cfa_offset_sf,
+      dwarf::DW_CFA_val_offset,
+      dwarf::DW_CFA_val_offset_sf,
+      dwarf::DW_CFA_val_expression,
+      dwarf::DW_CFA_MIPS_advance_loc8,
+      dwarf::DW_CFA_GNU_window_save,
+      dwarf::DW_CFA_AARCH64_negate_ra_state,
+      dwarf::DW_CFA_GNU_args_size};
+
+  dwarf::CIE TestCIE = createCIE(/*IsDWARF64=*/false,
+                                 /*Offset=*/0x0,
+                                 /*Length=*/0xff);
 
+  // See DWARF standard v3, section 7.23: low 6 bits are used to encode an
+  // extended opcode.
+  for (uint8_t Code = 0; Code <= 63; ++Code) {
+    if (ValidExtendedOpcodes.count(Code))
+      continue;
+
+    EXPECT_THAT_ERROR(parseCFI(TestCIE, Code),
+                      FailedWithMessage(("invalid extended CFI opcode 0x" +
+                                         Twine::utohexstr(Code))
+                                            .str()
+                                            .c_str()));
+  }
+}
+
+// Here we test how truncated Call Frame Instructions are parsed.
+TEST(DWARFDebugFrame, ParseTruncatedCFITest) {
   dwarf::CIE TestCIE = createCIE(/*IsDWARF64=*/false,
                                  /*Offset=*/0x0,
                                  /*Length=*/0xff);
 
   // Having an empty instructions list is fine.
-  EXPECT_THAT_ERROR(ParseCFI(TestCIE, {}), Succeeded());
+  EXPECT_THAT_ERROR(parseCFI(TestCIE, {}), Succeeded());
 
   // Unable to read an opcode, because the instructions list is empty, but we
   // say to the parser that it is not.
   EXPECT_THAT_ERROR(
-      ParseCFI(TestCIE, {}, /*Size=*/1),
+      parseCFI(TestCIE, {}, /*Size=*/1),
       FailedWithMessage(
           "unexpected end of data at offset 0x0 while reading [0x0, 0x1)"));
 
   // Unable to read a truncated DW_CFA_offset instruction.
   EXPECT_THAT_ERROR(
-      ParseCFI(TestCIE, {dwarf::DW_CFA_offset}),
+      parseCFI(TestCIE, {dwarf::DW_CFA_offset}),
       FailedWithMessage("unable to decode LEB128 at offset 0x00000001: "
                         "malformed uleb128, extends past end"));
 
   // Unable to read a truncated DW_CFA_set_loc instruction.
   EXPECT_THAT_ERROR(
-      ParseCFI(TestCIE, {dwarf::DW_CFA_set_loc}),
+      parseCFI(TestCIE, {dwarf::DW_CFA_set_loc}),
       FailedWithMessage(
           "unexpected end of data at offset 0x1 while reading [0x1, 0x9)"));
 
   // Unable to read a truncated DW_CFA_advance_loc1 instruction.
   EXPECT_THAT_ERROR(
-      ParseCFI(TestCIE, {dwarf::DW_CFA_advance_loc1}),
+      parseCFI(TestCIE, {dwarf::DW_CFA_advance_loc1}),
       FailedWithMessage(
           "unexpected end of data at offset 0x1 while reading [0x1, 0x2)"));
 
   // Unable to read a truncated DW_CFA_advance_loc2 instruction.
   EXPECT_THAT_ERROR(
-      ParseCFI(TestCIE, {dwarf::DW_CFA_advance_loc2}),
+      parseCFI(TestCIE, {dwarf::DW_CFA_advance_loc2}),
       FailedWithMessage(
           "unexpected end of data at offset 0x1 while reading [0x1, 0x3)"));
 
   // Unable to read a truncated DW_CFA_advance_loc4 instruction.
   EXPECT_THAT_ERROR(
-      ParseCFI(TestCIE, {dwarf::DW_CFA_advance_loc4}),
+      parseCFI(TestCIE, {dwarf::DW_CFA_advance_loc4}),
       FailedWithMessage(
           "unexpected end of data at offset 0x1 while reading [0x1, 0x5)"));
 
   // A test for an instruction with a single ULEB128 operand.
   auto CheckOp_ULEB128 = [&](uint8_t Inst) {
     EXPECT_THAT_ERROR(
-        ParseCFI(TestCIE, Inst),
+        parseCFI(TestCIE, Inst),
         FailedWithMessage("unable to decode LEB128 at offset 0x00000001: "
                           "malformed uleb128, extends past end"));
   };
 
   for (uint8_t Inst :
        {dwarf::DW_CFA_restore_extended, dwarf::DW_CFA_undefined,
         dwarf::DW_CFA_same_value, dwarf::DW_CFA_def_cfa_register,
         dwarf::DW_CFA_def_cfa_offset, dwarf::DW_CFA_GNU_args_size})
     CheckOp_ULEB128(Inst);
 
   // Unable to read a truncated DW_CFA_def_cfa_offset_sf instruction.
   EXPECT_THAT_ERROR(
-      ParseCFI(TestCIE, {dwarf::DW_CFA_def_cfa_offset_sf}),
+      parseCFI(TestCIE, {dwarf::DW_CFA_def_cfa_offset_sf}),
       FailedWithMessage("unable to decode LEB128 at offset 0x00000001: "
                         "malformed sleb128, extends past end"));
 
   // A test for an instruction with two ULEB128 operands.
   auto CheckOp_ULEB128_ULEB128 = [&](uint8_t Inst) {
     EXPECT_THAT_ERROR(
-        ParseCFI(TestCIE, Inst),
+        parseCFI(TestCIE, Inst),
         FailedWithMessage("unable to decode LEB128 at offset 0x00000001: "
                           "malformed uleb128, extends past end"));
 
     EXPECT_THAT_ERROR(
-        ParseCFI(TestCIE, {Inst, /*Op1=*/0}),
+        parseCFI(TestCIE, {Inst, /*Op1=*/0}),
         FailedWithMessage("unable to decode LEB128 at offset 0x00000002: "
                           "malformed uleb128, extends past end"));
   };
 
   for (uint8_t Inst : {dwarf::DW_CFA_offset_extended, dwarf::DW_CFA_register,
                        dwarf::DW_CFA_def_cfa, dwarf::DW_CFA_val_offset})
     CheckOp_ULEB128_ULEB128(Inst);
 
   // A test for an instruction with two operands: ULEB128, SLEB128.
   auto CheckOp_ULEB128_SLEB128 = [&](uint8_t Inst) {
     EXPECT_THAT_ERROR(
-        ParseCFI(TestCIE, Inst),
+        parseCFI(TestCIE, Inst),
         FailedWithMessage("unable to decode LEB128 at offset 0x00000001: "
                           "malformed uleb128, extends past end"));
 
     EXPECT_THAT_ERROR(
-        ParseCFI(TestCIE, {Inst, /*Op1=*/0}),
+        parseCFI(TestCIE, {Inst, /*Op1=*/0}),
         FailedWithMessage("unable to decode LEB128 at offset 0x00000002: "
                           "malformed sleb128, extends past end"));
   };
 
   for (uint8_t Inst : {dwarf::DW_CFA_offset_extended_sf,
                        dwarf::DW_CFA_def_cfa_sf, dwarf::DW_CFA_val_offset_sf})
     CheckOp_ULEB128_SLEB128(Inst);
 
   // Unable to read a truncated DW_CFA_def_cfa_expression instruction.
   EXPECT_THAT_ERROR(
-      ParseCFI(TestCIE, {dwarf::DW_CFA_def_cfa_expression}),
+      parseCFI(TestCIE, {dwarf::DW_CFA_def_cfa_expression}),
       FailedWithMessage("unable to decode LEB128 at offset 0x00000001: "
                         "malformed uleb128, extends past end"));
   EXPECT_THAT_ERROR(
-      ParseCFI(TestCIE, {dwarf::DW_CFA_def_cfa_expression,
+      parseCFI(TestCIE, {dwarf::DW_CFA_def_cfa_expression,
                          /*expression length=*/0x1}),
       FailedWithMessage(
           "unexpected end of data at offset 0x2 while reading [0x2, 0x3)"));
   // The DW_CFA_def_cfa_expression can contain a zero length expression.
-  EXPECT_THAT_ERROR(ParseCFI(TestCIE, {dwarf::DW_CFA_def_cfa_expression,
+  EXPECT_THAT_ERROR(parseCFI(TestCIE, {dwarf::DW_CFA_def_cfa_expression,
                                        /*ExprLen=*/0}),
                     Succeeded());
 
   // A test for an instruction with three operands: ULEB128, expression length
   // (ULEB128) and expression bytes.
   auto CheckOp_ULEB128_Expr = [&](uint8_t Inst) {
     EXPECT_THAT_ERROR(
-        ParseCFI(TestCIE, {Inst}),
+        parseCFI(TestCIE, {Inst}),
         FailedWithMessage("unable to decode LEB128 at offset 0x00000001: "
                           "malformed uleb128, extends past end"));
     EXPECT_THAT_ERROR(
-        ParseCFI(TestCIE, {Inst, /*Op1=*/0}),
+        parseCFI(TestCIE, {Inst, /*Op1=*/0}),
         FailedWithMessage("unable to decode LEB128 at offset 0x00000002: "
                           "malformed uleb128, extends past end"));
     // A zero length expression is fine
-    EXPECT_THAT_ERROR(ParseCFI(TestCIE, {Inst,
+    EXPECT_THAT_ERROR(parseCFI(TestCIE, {Inst,
                                          /*Op1=*/0, /*ExprLen=*/0}),
                       Succeeded());
     EXPECT_THAT_ERROR(
-        ParseCFI(TestCIE, {Inst,
+        parseCFI(TestCIE, {Inst,
                            /*Op1=*/0, /*ExprLen=*/1}),
         FailedWithMessage(
             "unexpected end of data at offset 0x3 while reading [0x3, 0x4)"));
   };
 
   for (uint8_t Inst : {dwarf::DW_CFA_expression, dwarf::DW_CFA_val_expression})
     CheckOp_ULEB128_Expr(Inst);
 }
 
 } // end anonymous namespace