[PATCH] D147271: [DebugInfo] Add DW_OP_LLVM_user extension point

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added subscribers: jmorse, aprantl.
dblaikie added a comment.
This revision is now accepted and ready to land.

Seems OK to me - wouldn't mind a second set of eyes. @jmorse @aprantl



================
Comment at: llvm/include/llvm/BinaryFormat/Dwarf.def:893
+// registered here it cannot be removed nor have its encoding changed. The
+// encoding space must skip zero (which is reserved) and have no gaps.
+//
----------------
scott.linder wrote:
> dblaikie wrote:
> > Why is zero reserved? (other than to add a the nop op, so there's something to test before we have any USEROPs implemented?)
> It was because the encoding for official operations in DWARF completely skips over byte "0", i.e. Table 7.9 in https://dwarfstd.org/doc/DWARF5.pdf begins:
> 
> 
> 
> 
> 
> # Table 7.9: DWARF operation encodings
> | Operation | No. of Operands | Operation Code | Notes |
> | Reserved | 0x01 | - | |
> | Reserved | 0x02 | - | |
> | DW_OP_addr | 0x03 | 1 | constant address (size is target specific) |
> 
> I don't see a particular reason to skip it, but there is also very little lost. I even considered reserving all of the 1-byte encodings, just so there is less of a "early users get a more compact encoding" effect.
> 
> I am also fine with just using 0 for the `nop` operation
Ah, fair enough - yeah - maybe using zero would be nice then? 


================
Comment at: llvm/lib/BinaryFormat/Dwarf.cpp:175-208
+static StringRef LlvmUserOperationEncodingString(unsigned Encoding) {
+  switch (Encoding) {
+  default:
+    llvm_unreachable("unhandled DWARF operation with LLVM user op");
+#define HANDLE_DW_OP_LLVM_USEROP(ID, NAME)                                     \
+  case DW_OP_LLVM_##NAME:                                                      \
+    return "DW_OP_LLVM_" #NAME;
----------------
scott.linder wrote:
> dblaikie wrote:
> > Hmm, any chance we could condense these into a bigger macro (maybe another .def file) that stamps out all these operations, since we have to stamp these out for each kind of enum?)
> I'm not sure I understand what you mean; do you mean an improvement over the existing support that could land as NFC before my changes?
Yep - though I'm not sure it's possible (macro can't have #include in it, I guess, so not sure - oh, maybe it'd be another .def file... which takes, via macro definitions, the various name spellings, and then stamps out the lookup/string/etc functions)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147271