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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/MC/MCAsmInfo.h:395-396
+
+  /// True if the target needs dwarf section length in the header(if any) of
+  /// dwarf section in assembly file. Defaults to true.
+  bool DwarfSectionSizeRequired = true;
----------------
Minor comment wording nits.


================
Comment at: llvm/include/llvm/MC/MCAsmInfo.h:682
+
+  bool requiredDwarfSectionSize() const {
+    return DwarfSectionSizeRequired;
----------------
Rename to clarify that this returns a `bool` re: whether the size is required as opposed to a value for a size that is required.


================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:50-52
+    assert(
+        (ST == XCOFF::XTY_SD || ST == XCOFF::XTY_CM || ST == XCOFF::XTY_ER) &&
+        "Invalid or unhandled type for csect.");
----------------
I don't think we want ersatz csect symbol properties for the DWARF sections. It makes the symbol table management fragile. At the same time, we don't want an ersatz XCOFF section flags field for csects. I suggest grouping the csect properties together and making them optional. Same with making the section flags optional (all bits zero has a valid interpretation).


================
Comment at: llvm/include/llvm/MC/MCStreamer.h:1099
+  /// raw debug line contents.
+  virtual void emitDwarfAdvanceLineAddr(int64_t LineDelta,
+                                        const MCSymbol *LastLabel,
----------------
Should the override in `MCObjectStreamer` be marked in this patch?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp:211-213
+  // If the assembler on some targets fills the dwarf unit length, we don't need
+  // to emit the length in compiler. For exmaple AIX assembler requires the
+  // assembly file without unit length in debug sections header.
----------------
Minor comment wording nits.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp:225-227
+  // If the assembler on some targets fills the dwarf unit length, we don't need
+  // to emit the length in compiler. For exmaple AIX assembler requires the
+  // assembly file without unit length in debug sections header.
----------------
Same comment.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:360-364
+  // If the assembler on some targets will fill the dwarf unit length, we
+  // don't need to emit the length in compiler. For exmaple AIX assembler
+  // requires an assembly file without unit length in debug sections header.
+  // We need to adjust the reference here to contain the assembler insertion
+  // length.
----------------
Comment wording change suggestion.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:387-390
+  // 1: ELF.
+  // 2: XCOFF. AIX assembler will fill debug sections length according to
+  //    DWARF64 format for 64 bit assembly, so must use DWARF64 in compiler too
+  //    for 64 bit mode.
----------------
Comment wording change suggestion.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:391-392
+  //    for 64 bit mode.
+  Dwarf64 &= (Asm->TM.Options.MCOptions.Dwarf64 && TT.isOSBinFormatELF()) ||
+             TT.isOSBinFormatXCOFF();
 
----------------
This seems to be missing a `report_fatal_error` in the case where XCOFF64 is being used and DWARF64 is not selected (perhaps due to options that select for DWARF version 2).


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:1890
 
+// Generate dwarf line sections for assembly mode without .loc/.file
+void MCAsmStreamer::emitDwarfAdvanceLineAddr(int64_t LineDelta,
----------------
Minor nit: let's be consistent in this patch with using "DWARF".


================
Comment at: llvm/lib/MC/MCDwarf.cpp:488-490
+  // If the assembler on some targets will fill the dwarf unit length, we
+  // don't need to emit the length in compiler. For exmaple AIX assembler
+  // requires the assembly file without unit length in debug sections header.
----------------
Same replacement as elsewhere.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCAsmInfo.cpp:70
+
+  // Set up dwarf DIRECTIVES
+  MinInstAlignment = 4;
----------------
Use more conservative formatting from `PPCELFMCAsmInfo`.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2276-2277
 bool PPCAIXAsmPrinter::doFinalization(Module &M) {
+  // Add text end before we do any finalization. This is used to tell debug line
+  // section where the text end is.
+  if (isa<MCAsmStreamer>(OutStreamer) &&
----------------
Minor nit: add missing "the".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95518