[PATCH] D76878: Implement DW_{OP,AT}_LLVM_* for Heterogeneous Debugging

[Venkata Ramanaiah Nalamothu via Phabricator via llvm-commits]

RamNalamothu added a comment.

@djtodoro thanks for the quick response



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1056
 
+  if (useHeterogeneousExtensionAttributes()) {
+    NewCU.addString(Die, dwarf::DW_AT_LLVM_augmentation, "[amdgpu:v0.1]");
----------------
djtodoro wrote:
> no need for the curly brackets here
Done.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.h:750
+  /// Returns whether extensions defined at
+  /// https://llvm.org/docs/AMDGPUDwarfProposalForHeterogeneousDebugging.html
+  /// are enabled.
----------------
djtodoro wrote:
> I'd say that the link was mentioned too many times.
I got rid of the redundant ones. And the rest, unless they are big annoyance, I feel they provide ready access and inform what is being supported.


================
Comment at: llvm/lib/MC/MCDwarf.cpp:1662
+  if (MAI->supportsHeterogeneousDebuggingExtensions())
+    Augmentation += "[llvm:v0.0]";
+  Streamer.emitBytes(Augmentation);
----------------
djtodoro wrote:
> clayborg wrote:
> > Should we use a single character here? If we don't, unwinders might end up parsing each character individually and that might make unwinders fail as they parse each character. Or does the unwind spec state that everything between square brackets is a single entry? This seems dangerous unless the spec has some rule like this.
> @RamNalamothu have you taken a look into this?
The thinking behind the multi-character versioning is:
  - we should be able to distinguish multiple versions from multiple vendors because
      #   some encoding spaces, e.g. DW_OP_*, are too short and the available encodings are quickly occupied leading to collisions
      #   many historic encodings are not being removed from the code base even when they are not being generated from the compiler
  - it's better to follow a standard versioning such as https://semver.org

I don't think all of the above could be addressed using a single character versioning in a meaningful clean manner, if at all it can be done. And the spec can enforce what is needed from all the interfacing tools.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76878