[PATCH] D89845: Add the ability to extract the unwind rows from DWARF Call Frame Information.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:448
+  /// Get a DWARF CFI call frame string for the given DW_CFA opcode.
+  StringRef CallFrameString(unsigned Opcode) const;
+
----------------



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:575
+    case dwarf::DW_CFA_offset:
+      LLVM_FALLTHROUGH;
+    case dwarf::DW_CFA_offset_extended:
----------------
I might be wrong, but I think `LLVM_FALLTHROUGH` wasn't needed if there were multiple labels with identical behaviour. I think it is for the case where you have:
```
case 1:
  something();
  // don't break
  LLVM_FALLTHROUGH;
case 2:
  somethingThatCase1AlsoNeedsToDo();
  break;
```


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:617
+        // should be initially set to 1.
+        constexpr uint32_t AARrch64DWARFPAuthRaState = 34;
+        auto LRLoc = Row.getRegisterLocations().getRegisterLocation(
----------------



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:627
+                errc::invalid_argument,
+                "%s encountered when existing rule for this registrer is not "
+                "a constant",
----------------



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:368
+    return createStringError(std::errc::invalid_argument,
+                             "Operand index %u is not valid.", OperandIdx);
+  OperandType Type = CFIP.getOperandTypes()[Opcode][OperandIdx];
----------------
jhenderson wrote:
> These messages should all use `PRIu32` to go with the `uint32_t` type.
> 
> Also, the style for error messages according to the style guide nowadays is no capitalization on first letter (for regular words at least) and no trailing full stop.
> Also, the style for error messages according to the style guide nowadays is no capitalization on first letter (for regular words at least) and no trailing full stop.

Looks like you missed the second half of my previous comment?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:432
+  switch (Type) {
+  case OT_Unset:
+    return createStringError(std::errc::invalid_argument,
----------------
jhenderson wrote:
> Some of these cases are identical. Can we combine them? Same goes above in `getOperandAsUnsigned`.
This comment hasn't been addressed? If you add a function that maps an operand type to a string (or use an existing one if there is one), you could fold several of these cases together by using that function.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89845/new/

https://reviews.llvm.org/D89845