[PATCH] D76878: Implement new DW_OP_LLVM_* operations

[Scott Linder via Phabricator via llvm-commits]

scott.linder marked 2 inline comments as done.
scott.linder added a comment.

Thank you for the review, and sorry for neglecting to include any context. The proposal which defines these new operations is at https://llvm.org/docs/AMDGPUUsage.html#dwarf. @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.



================
Comment at: llvm/include/llvm/BinaryFormat/Dwarf.def:670
+// FIXME: These collide with some HP and PGI vendor extensions.
+HANDLE_DW_OP(0xe1, LLVM_form_aspace_address, 4, LLVM)
+HANDLE_DW_OP(0xe2, LLVM_push_lane, 4, LLVM)
----------------
aprantl wrote:
> aprantl wrote:
> > djtodoro wrote:
> > > Should the Vendor be AMD (or AMDGPU) instead of LLVM? Or this is only LLVM related.
> > > 
> > > What about other tools, such as GDB, do they have the support for this?
> > According to libdwarf these numbers are already taken by HP extensions. I don't know how much this practically matters, but it would be good if we imported all the various vendor extension ranges from libdwarf and binutils...
> > Should the Vendor be AMD (or AMDGPU) instead of LLVM? Or this is only LLVM related.
> 
> If the extensions are only useful for AMD hardware then that would be appropriate. If they are generally useful the LLVM vendor might make more sense.
These extensions are generally useful, and we are working on implementing them in GDB (currently in an open-source fork rather than directly upstream, although our intention is to upstream as feasible) in parallel with implementing them in LLVM so we have an initial implementation to prove the concept.

The DW_OP_ user encodings have become pretty cramped, as operations are added but never removed, even when they are eventually upstreamed into a version of DWARF. I know that e.g. AArch64 has elected to reserve one encoding to represent a "family" of instructions, where the first operand is the true "AArch64 opcode", but that comes with a lot of infrastructure work and either means we have to break the invariant that operations have a fixed number of opcodes or we have to waste a lot of space in the final encoding with un-used operands.

Our current intention is to instead implement the `DW_AT_LLVM_augmentation` described at https://llvm.org/docs/AMDGPUUsage.html#debugging-information-entry-attributes as a temporary way to select which of the overloaded versions of these user opcodes should be used for a given DIE.


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/amdgpu_proposal.s:26-27
+ .cfi_escape 0x10, 0x00, 0x02, 0xe6, 0x00
+ # CHECK-NEXT: DW_CFA_expression: reg0 DW_OP_LLVM_undefined
+ .cfi_escape 0x10, 0x00, 0x01, 0xe7
+ # CHECK-NEXT: DW_CFA_expression: reg0 DW_OP_LLVM_aspace_bregx EAX+2
----------------
dstenb wrote:
> In which ways does this differ from `DW_CFA_undefined reg0`?
It is not a CFA-only rule, it is just a new dwarf expression operation. DW_OP_LLVM_undefined pushes an "undefined location description" on the stack, which is a new concept that was needed in order to generalize the DWARF stack to support operating on location descriptions. It is a way to explicitly capture the existing rule where two adjacent `DW_OP_piece`-like operations result in some undefined bits in the final composite location description. @t-tye can probably do a better job describing how it relates to the `undefined` CFA rule in the new proposal; I think in this case the two forms are equivalent, as they will both result in the unwound register location description being an undefined location description.

I am only using `.cfi_escape` here as a hacky way to generate some DWARF expressions in a way that dwarfdump will parse, i.e. none of these expressions have to be reasonable things one would want to emit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76878