[PATCH] D76162: [AIX] discard the label in the csect of function description and use qualname for linkage

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1972
 MCSection *TargetLoweringObjectFileXCOFF::getSectionForFunctionDescriptor(
-    const MCSymbol *FuncSym) const {
-  return getContext().getXCOFFSection(FuncSym->getName(), XCOFF::XMC_DS,
-                                      XCOFF::XTY_SD, XCOFF::C_HIDEXT,
-                                      SectionKind::getData());
+    const GlobalObject *GO, const MCSymbol *FuncSym) const {
+  return getContext().getXCOFFSection(
----------------
DiggerLin wrote:
> jasonliu wrote:
> > MCSymbolXCOFF already have a getStorageClass() function, I don't think we need to pass in GlobalObject to get it. If we need to, then we get the wrong FuncSym.
> we do not set StorageClass() for the FuncSym before we call the getSectionForFunctionDescriptor. We can not get getStorageClass() before set it.
I think we should modify getSectionForFunctionDescriptor to only take `const Function *` as argument instead. You could get everything you need from that alone. 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1549
+
+  if (XCOFFSym->hasContainingCsect()) {
+    MCSymbolXCOFF *QualName =
----------------
DiggerLin wrote:
> jasonliu wrote:
> > This is similar issues I saw above, I think we have the wrong MCSymbol here. If we have the correct one, we could just call emitLinkage without this if statement.
> when we do to the emitLinkage in the doFinalization()
> there is function .llvm.StackProtector which do not have  hasContainingCsect()
What I meant is, GVSym we received from function argument, should be foo[DS], or foo depending which one we need. We shouldn't try to get the correct one here. It's the caller's responsibility to pass in the correct MCSymbol. And in this patch, it's not even necessary to override emitLinkage for AIX. 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1562
   // Get the function descriptor symbol.
   CurrentFnDescSym = getSymbol(&MF.getFunction());
   // Set the alignment and the containing csect.
----------------
CurrentFnDescSym is `foo` here, which is wrong. It should be `foo[DS]`.
You can achieve that by moving this after getting FnDescSec:
`CurrentFnDescSym = FnDescSec->getQualNameSymbol();`



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76162