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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
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.
+//
----------------
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


================
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;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147271