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

[ChenZheng via Phabricator via llvm-commits]

shchenz marked 9 inline comments as done.
shchenz added inline comments.


================
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.");
----------------
hubert.reinterpretcast wrote:
> 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).
Good idea. I created one NFC patch D95931


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


================
Comment at: llvm/lib/MC/MCDwarf.cpp:492
+  if (!(!MCOS->getContext().getAsmInfo()->requiredDwarfSectionSize() &&
+        isa<MCAsmStreamer>(MCOS))) {
+    if (context.getDwarfFormat() == dwarf::DWARF64)
----------------
ikudrin wrote:
> The code breaks the abstraction of `MCAsmStream` and calculates `PreHeaderLengthBytes` differently depending on the concrete implementation of the base class. Yes, that is much simpler to write, but much harder to understand, because the semantic of `PreHeaderLengthBytes` becomes vague, resulting in weird and misleading code. I believe that should be avoided at all costs.
Thanks for your high-level comments. Indeed, checking the streamer type inside general debug code makes no sense. I have updated them according to your comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95518