[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).
----------------
RamNalamothu wrote:
> 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?
> 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.

UnwindLocation does get used for both CFA and register values, and yes a UnwindLocation does have more values for registers, but since we since CFA values are parsed using special opcodes, there should be no need to worry as those are what creates the right locations. 

That being said, I think moving the address space into UnwindLocation seems like the right thing to do here. Nothing that parses the regular register locations should be adding an address space to the location, though I could see this being needed for some registers locations in the future.

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

I believe the LLDB unwind code does this but I feel this splits up the location code a bit since the CFA usually is register + offset or DWARF expression which are both valid register rules. You could make a CFALocation that inherits from UnwindLocation and adds the address space if that makes things more clear? Then you can also add some asserts when someone tries to set the location to something that the DWARF spec doesn't allow for CFA values.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76877