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

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
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);
----------------
jasonliu wrote:
> shchenz wrote:
> > 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.
> I think what Hubert meant is to make a call to `getXCOFFSection` inside of `getXCOFFDwarfSection` so that you don't need to pass in so many default parameters when you just want to get the Dwarf Section. i.e. The getXCOFFDwarfSection signature could be 
> ```
> MCSectionXCOFF *
> getXCOFFDwarfSection(StringRef Section, XCOFF::DwarfSectionSubtypeFlags DwarfSubtypeFlags, bool MultiSymbolsAllowed = true, const char *BeginSymName = nullptr);
> ```
Thanks for the review @jasonliu 

That would make the API more confusing in my opinion. For that case, we have `getXCOFFDwarfSection` and `getXCOFFSection`. DWARF section should also be an XCOFF section, so why can't we call `getXCOFFSection` directly for DWARF section? `getXCOFFSection` obviously already has a parameter for DWARF section?


I am a little in favor of the current implementation, if you guys insist, I will do it in another NFC patch.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1804-1807
+  // When -function-sections is enabled, generating function section end symbol
+  // to tell debug line section where is the end of current function section.
+  if (TM.getFunctionSections() && !MAI->usesDwarfFileAndLocDirectives() &&
+      MMI->hasDebugInfo())
----------------
hubert.reinterpretcast wrote:
> @jasonliu, I'd like your analysis of how this interacts with explicit sections.
`explicit sections` are generated when `-fdata-sections` is enabled? If so, I think debugging functionality should not be impacted. We only need to worry about the "text" section for debug line. For `explicit sections` functionality, I guess it should not be impacted too as we only add new labels, not data, like `vbyte`, `byte`

Anyway, your input is appreciated @jasonliu 


================
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:
> shchenz wrote:
> > 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?
> I just meant that https://reviews.llvm.org/D95518?id=326625#inline-915279 applies here too. I'm not aware of a functionality issue with this.
Ah, OK, I was thinking you were saying "comments" in llvm source code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95518