[PATCH] D76877: Implement DW_CFA_LLVM_* for Heterogeneous Debugging

[Greg Clayton via Phabricator via llvm-commits]

clayborg added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:241
   Optional<uint64_t> Address; ///< Address for row in FDE, invalid for CIE.
+  uint32_t CFAAddressSpace = 0; /// The address space for the CFA address.
   UnwindLocation CFAValue;    ///< How to unwind the Call Frame Address (CFA).
----------------
Is this the address segment for the address of the row? If so we should change "Address" above to be a:
```
Optional<object::SectionedAddress> Address;
```
If this address space qualifies the CFAValue below, then we should move it into UnwindLocation as a "Optional<uint32_t> AddressSpace;" variable


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:260
+  /// This represents the address space the address of this row is located in.
+  uint8_t getCFAAddressSpace() const { return CFAAddressSpace; }
+
----------------
You are returning uint8_t but storing this as a CFAAddressSpace as a uint32_t above.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:404
 public:
-  typedef SmallVector<uint64_t, 2> Operands;
+  typedef SmallVector<uint64_t, 3> Operands;
 
----------------
Make a constexpr for MaxOperands and use it everywhere so if we bump this number again, we don't need to modify a bunch of places in the code.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:176
+  if (CFAAddressSpace != 0)
+    OS << " as" << CFAAddressSpace;
   if (RegLocs.hasLocations()) {
----------------
not sure printing something like "as2" will be easy for people to know that this means "address space 2". Maybe "address space 2" would be better?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:406
                                               uint32_t OperandIdx) const {
-  if (OperandIdx >= 2)
+  if (OperandIdx >= 3)
     return createStringError(errc::invalid_argument,
----------------



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:451
                                             uint32_t OperandIdx) const {
-  if (OperandIdx >= 2)
+  if (OperandIdx >= 3)
     return createStringError(errc::invalid_argument,
----------------



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:839
 
-  return ArrayRef<OperandType[2]>(&OpTypes[0], DW_CFA_restore+1);
+  return ArrayRef<OperandType[3]>(&OpTypes[0], DW_CFA_restore + 1);
 }
----------------



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:847
                               uint64_t Operand) const {
-  assert(OperandIdx < 2);
+  assert(OperandIdx < 3);
   uint8_t Opcode = Instr.Opcode;
----------------



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:895
+  case OT_AddressSpace:
+    OS << format(" as%" PRId64, Operand);
+    break;
----------------
" address space %"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76877