[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: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).
----------------
clayborg wrote:
> 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
> 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: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).
----------------
RamNalamothu wrote:
> clayborg wrote:
> > 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
> > 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
> 
> 
No, the address space here qualifies the CFA and specifies, among the different address spaces the target supports, in which address space the relevant stack frame (CFA location) resides. For the sake of understanding this, think of the address spaces as different memory regions, with unique characteristics.

Thought about moving it into `UnwindLocation` but I noticed that `UnwindLocation` is being used by both CFA and register rules which, theoretically, making it possible to say CFA's location is `CFAPlusOffset` and also looks like there aren't enough checks to catch this scenario. Being not sure, I have added it to `UnwindRow` which has CFAValue.

Does it make sense to split `UnwindLocation` suitably, so that CFA and register rules could use only what is possible as per DWARF spec?


================
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; }
+
----------------
clayborg wrote:
> You are returning uint8_t but storing this as a CFAAddressSpace as a uint32_t above.
Typo, will fix it. Thank you.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:404
 public:
-  typedef SmallVector<uint64_t, 2> Operands;
+  typedef SmallVector<uint64_t, 3> Operands;
 
----------------
clayborg wrote:
> 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.
Will change it.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:176
+  if (CFAAddressSpace != 0)
+    OS << " as" << CFAAddressSpace;
   if (RegLocs.hasLocations()) {
----------------
clayborg wrote:
> 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?
Yes.

How about "addrspace 2", which is similar to what LLVM IR/MIR uses?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76877