[PATCH] D76877: Implement DW_CFA_LLVM_* for Heterogeneous Debugging

Fri Oct 9 09:47:40 PDT 2020

MaskRay added a comment.

In D76877#1946283 <https://reviews.llvm.org/D76877#1946283>, @scott.linder wrote:

> The new operation is defined at https://llvm.org/docs/AMDGPUUsage.html#cfa-definition-instructions. @t-tye is working on formatting the proposal to be sent to upstream DWARF as an RFC, and I am working to land these patches as an initial implementation.

Can you link the documentation in the summary? Currently https://llvm.org/docs/AMDGPUUsage.html does not mention DW_CFA_LLVM_

================
Comment at: llvm/lib/MC/MCDwarf.cpp:1446
+    unsigned Reg = Instr.getRegister();
+    unsigned AddressSpace = Instr.getAddressSpace();
+    Streamer.emitIntValue(dwarf::DW_CFA_LLVM_def_aspace_cfa, 1);
----------------
The variables are used only once and can be inlined

================
Comment at: llvm/lib/MC/MCDwarf.cpp:1449
+    Streamer.emitULEB128IntValue(Reg);
+    CFAOffset = -Instr.getOffset();
+    Streamer.emitULEB128IntValue(CFAOffset);
----------------
The negative offset looks strange. 

Before May, there was a double negation problem which has been fixed by 0840d725c4e7c98bb440db7b886054525be6ddf1

================
Comment at: llvm/test/MC/ELF/cfi-llvm-def-aspace-cfa-errors.s:7
+
+// CHECK: [[@LINE+1]]:{{[0-9]+}}: error: invalid register name
+.cfi_llvm_def_aspace_cfa foo
----------------
`[[@LINE]]` is deprecated syntax. Please use `[[#@LINE+1]]` for new tests.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76877