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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

> in assembly path

I was a bit confused by this term. You may just use MCAsmStreamer.

Hi, I have followed some part of the patch, mainly related to `MCDwarfLineEntry::Make`. Can you please explain a bit why .file/.loc cannot be used for assembler output?
If you have desire to implement .file/.loc for integrated assemblers, you will not need alternative paths to .file/.loc .
If you have other tools and they cannot consume .file/.loc, please make it clear.



================
Comment at: llvm/include/llvm/MC/MCDwarf.h:176
   // has yet to have a line entry made for it is made.
-  static void Make(MCObjectStreamer *MCOS, MCSection *Section);
+  static void Make(MCStreamer *MCOS, MCSection *Section);
 };
----------------
Make it lowercase BTW.


================
Comment at: llvm/include/llvm/MC/MCDwarf.h:314
   // This emits the Dwarf file and the line tables for all Compile Units.
-  static void Emit(MCObjectStreamer *MCOS, MCDwarfLineTableParams Params);
+  static void Emit(MCStreamer *MCOS, MCDwarfLineTableParams Params);
 
----------------
Make it lowercase


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:394
+
+  if (!Dwarf64 && TT.isArch64Bit() && TT.isOSBinFormatXCOFF())
+    report_fatal_error("XCOFF requires DWARF64 for 64-bit mode!");
----------------
The report_fatal_error only triggers if `DwarfVersion < 3`. I think the driver and llvm-mc should surface the error and essentially make this line unreachable: then due to whether we have an error subsequently, this report_fatal_error may not be needed.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1074
+  // Target doesn't support .loc/.file directives, return early.
+  if (!MAI->usesDwarfFileAndLocDirectives())
+    return FileNo;
----------------
This can be combined with the previous if. The comment can be added below, along the lines of: For a new file, print a .file directive if the target supports it.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1104
+  // Target doesn't support .loc/.file directives, return early.
+  if (!MAI->usesDwarfFileAndLocDirectives())
+    return;
----------------
Drop this line - which is untested.

Does XCOFF support DWARF v5? MCObjectFileInfo does not have them. The binary format seems to hard code some section type values.



================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1125
+  if (!MAI->usesDwarfFileAndLocDirectives()) {
+    MCDwarfLineEntry::Make(this, getCurrentSectionOnly());
+    this->MCStreamer::emitDwarfLocDirective(FileNo, Line, Column, Flags, Isa,
----------------
Make is a no-op here.


================
Comment at: llvm/lib/MC/MCObjectFileInfo.cpp:906
+      ".dwabrev", SectionKind::getMetadata(), XCOFF::CsectProperty(0xFF, 0xFF),
+      /* MultiSymbolsAllowed*/ true, ".dwabrev",
+      XCOFF::STYP_DWARF | XCOFF::SSUBTYP_DWABREV);
----------------
`/*MultiSymbolsAllowed=*/true` or `/* MultiSymbolsAllowed */ true`


================
Comment at: llvm/test/DebugInfo/XCOFF/empty.ll:171
+; ASM32-NEXT:  L..info_string0:
+; ASM32-NEXT:   .byte   'c,'l,'a,'n,'g,' ,'v,'e,'r,'s,'i,'o,'n,' ,'1,'2,'.,'0,'.,'0,0000 # string offset=0
+; ASM32-NEXT:  L..info_string1:
----------------
Is .ascii or .asciz available?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95518