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

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

thanks for your review @hubert.reinterpretcast . See my inline reply.



================
Comment at: llvm/include/llvm/MC/MCContext.h:586-590
     getXCOFFSection(StringRef Section, SectionKind K,
                     Optional<XCOFF::CsectProperties> CsectProp = None,
                     bool MultiSymbolsAllowed = false,
-                    const char *BeginSymName = nullptr);
+                    const char *BeginSymName = nullptr,
+                    unsigned SecFlags = 0);
----------------
hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > Use `Optional` with a suitable enumeration type.
> It may make sense to add a `getXCOFFDwarfSection` function that forwards to this.
hmm, I would let this function like this for now. There would be many redundant codes between `getXCOFFDwarfSection` and `getXCOFFSection` if we create a new `getXCOFFDwarfSection`. Let me know if you think it is profitable to add a new function. Maybe we need a big NFC patch for this change.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:2351
+
+void MCAsmStreamer::doFinalizationAtTextEnd() {
+  // Emit text end. This is used to tell the debug line section where the text
----------------
hubert.reinterpretcast wrote:
> I tried this patch with `-ffunction-sections`, and the assembly I get generates a duplicate definition of `L...dwline`:
> ```
> 	.byte	3
> 	.byte	1
> 	.byte	1
> 	.csect .main[PR],2
> L..sec_end1:
> 
> 	.dwsect 0x20000
> L...dwline:
> 	.byte	0
> 	.byte	5
> 	.byte	2
> 	.vbyte	4, L..sec_end1
> 	.byte	0
> 	.byte	1
> 	.byte	1
> L..debug_line_end0:
> ```
Good catch. Fixed in the new update.


================
Comment at: llvm/lib/MC/MCSymbolXCOFF.cpp:30-32
-  assert(!getName().equals(getUnqualifiedName()) &&
-         "Symbol does not represent a csect; can only set a MCSectionXCOFF "
-         "representation for a csect.");
----------------
hubert.reinterpretcast wrote:
> This function (as named) is for csects. The assert should stay or some more extensive changes are needed.
I have added a FIXME in the caller of `setRepresentedCsect`, I will fix this later in another patch.


================
Comment at: llvm/test/DebugInfo/XCOFF/empty.ll:75-77
+; ASM32-NEXT:  L..func_end0:
+; ASM32-NEXT:                                          # -- End function
+; ASM32-NEXT:  L..sec_end0:
----------------
hubert.reinterpretcast wrote:
> See comment about the size of the traceback table and the resulting alignment consequences.
Can you help to point out which comments? Do you mean we will have a functionality issue if we don't align the text section?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95518