[PATCH] D76877: Implement DW_CFA_LLVM_* for Heterogeneous Debugging

[Venkata Ramanaiah Nalamothu via Phabricator via llvm-commits]

RamNalamothu added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:86-88
+  UnwindLocation(Location K, uint32_t Reg, int32_t Off, uint32_t AS, bool Deref)
+      : Kind(K), RegNum(Reg), Offset(Off), AddrSpace(AS), Dereference(Deref) {}
+
----------------
clayborg wrote:
> Revert and use above constructor
Ah, I didn't know earlier that `Optional<uint32_t> AS = None` is possible.

Done.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:110-111
   /// information.
   static UnwindLocation createIsCFAPlusOffset(int32_t Off);
   static UnwindLocation createAtCFAPlusOffset(int32_t Off);
   /// Create a location where the saved value is in (Deref == false) or at
----------------
clayborg wrote:
> You could add address space args to this as well, not required though
The current intent is that address space qualifies only the CFA and since `CFAPlusOffset` refers to just an offset from CFA, probably address space is not needed here.

Skipping it for now.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:136-139
+  uint32_t getAddressSpace() const {
+    assert(Kind == RegPlusOffsetInAddrSpace && AddrSpace.hasValue());
+    return *AddrSpace;
+  }
----------------
clayborg wrote:
> 
I would prefer to retain the assert to catch any possible unintended use of address space.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76877