[PATCH] D95518: [Debug-Info][XCOFF] support dwarf for XCOFF for assembly output

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:2323
+
+  // FIXME: use section end symbol as end of the Section. We need to consider
+  // the explicit sections and -ffunction-sections when we try to generate or
----------------
I hope this FIXME is not going to stay here for long (and we will quickly have a patch to address this). As this is really a workaround here and it creates different behavior between object file generation and assembly generation which is undesirable.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:2344
+    // Set to CU beginning.
+    emitIntValue(dwarf::DW_LNS_extended_op, 1);
+    emitULEB128IntValue(PointerSize + 1);
----------------
The assembly generated is hard for human to consume. It will be very difficult to debug if anything goes wrong between those lines. It would be great if we could use addComment to let people know what each of this byte means (It could be in a follow on patch if this is too much for this one). 
Some of the existing dwarf code does not really have those comments because they were meant for object file generation only. But now, it might make sense to have those comments available.
Arguably, one could use some other tools to dump the assembled object file and figure out what each entry means. But those are extra steps and it requires time to map the dump output from object file to the original asm output. 


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:2363
+
+    emitIntValue(0, 1);
+    emitULEB128IntValue(1);
----------------
Should use `dwarf::DW_LNS_extended_op` instead of hardcoding `0`?


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:2370-2373
+  emitIntValue(dwarf::DW_LNS_extended_op, 1);
+  emitULEB128IntValue(PointerSize + 1);
+  emitIntValue(dwarf::DW_LNE_set_address, 1);
+  emitSymbolValue(Label, PointerSize);
----------------
We have the same 4 lines on these 3 blocks. Do we want to common them up? 
Also, better yet, is it possible if we just use most of the logic from MCObjectStreamer::emitDwarfAdvanceLineAddr instead of making our own assembly version?
For example, put the common code that would work for both assembly and object file generation in a common function that could access by MCAsmStreamer and MCObjectStreamer. And in the override version of emitDwarfAdvanceLineAddr, we could call the common function and handle the differences between assembly and object file generation. Or, at least we want to see if we could reuse `MCDwarfLineAddr::Emit` which gets called in MCObjectStreamer::emitDwarfAdvanceLineAddr, instead of duplicating the logic here.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2341
+  if (!MAI->usesDwarfFileAndLocDirectives() && MMI->hasDebugInfo())
+    OutStreamer->doFinalizationAtSectionEnd(
+        OutStreamer->getContext().getObjectFileInfo()->getTextSection());
----------------
I wonder if `doFinalizationAtSectionEnd` really has to be in MCStreamer. At least for this patch, it could just be a helper static function, or private member function of PPCAIXAsmPrinter or even a lambda here, and no other target actually needs it. We could always refactor it to a larger scope later when we find the need for it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95518