[PATCH] D95518: [Debug-Info][XCOFF] support dwarf for XCOFF in assembly path
ChenZheng via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 4 20:28:55 PST 2021
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
More information about the llvm-commits
mailing list