[PATCH] D71125: [AIX] Avoid unset csect assert for functions defined after their use in TOC

[David Tenty via Phabricator via llvm-commits]

daltenty marked 2 inline comments as done.
daltenty added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1740
       CurrentFnDescSym->getName(), XCOFF::XMC_DS, XCOFF::XTY_SD,
-      XCOFF::C_HIDEXT, SectionKind::getData());
+      XCOFF::C_EXT, SectionKind::getData());
   cast<MCSymbolXCOFF>(CurrentFnDescSym)->setContainingCsect(FnDescSec);
----------------
hubert.reinterpretcast wrote:
> Xiangling_L wrote:
> > DiggerLin wrote:
> > > for global ,I checked the gcc generated Functions Description as HIDDEN.
> > > for static function, I checked both gcc and xlc generated Functions Description as HIDDEN.
> > For static function, Digger is right, function descriptor symbol should have internal storage type. 
> > 
> > But for external linkage function, I think what Digger mentioned `I checked the gcc generated Functions Description as HIDDEN` should be interpreted as `GCC generates two symbols for function descriptor, one is .csect foo[DS]`, the `SD` symbol has `C_HIDEXT` storage type, the other one is `foo` as a label within the csect which has `C_EXT` SC?  And the label one plays the important role in GCC case to expose foo's function descriptor when the address of function is taken.
> > 
> > So in LLVM,  since we use the same symbol to represent the `foo` in `.csect foo[DS]` and the `foo` as label, I think the storage class here should follow the linkage type of function symbol, like:
> > 
> > 
> > ```
> > const XCOFF::StorageClass SC =
> >             TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GO);
> > 
> >  MCSectionXCOFF *FnDescSec = OutStreamer->getContext().getXCOFFSection(
> >       CurrentFnDescSym->getName(), XCOFF::XMC_DS, XCOFF::XTY_SD,
> >       SC, SectionKind::getData());
> > ```
> I agree with @Xiangling_L. The storage class should match that of the function symbol itself.
I agree. I originally intended to use getStorageClassForGlobal, but for some reason the XCOFF object file format  document declares that CSects of mapping class have XMC_DS have storage class value of C_EXT. 

As noted this doesn't make much sense, so I will update this as described.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1905
+
+  if (GOKind.isText()) {
+    // If the MO is a function, we want to make sure to refer to the function
----------------
DiggerLin wrote:
> can we  implement as 
>  if (GOKind.isText()) {
>    if (!XSym->hasContainingCsect()) {
>       .......
>       XSym->setContainingCsect(Csect);
>     }
>  return XSym->getContainingCsect()->getQualNameSymbol();
> }
>  
I'm thinking we may need to refactor how we setup containing csects for GlobalObjects in general. We end up duplicating the logic for what type of csect to create for a particular type of global in multiple places, and its not clear who owns setting up the containing csect (i.e. is it the first place we need the csect, or emitGlobalVariable and friends?).

Perhaps getSymbol() could setup the containing CSect if it doesn't exist. Or maybe some type of getOrCreateContainingCsect(GlobalObject) function is more appropriate.

Anyway, I'd like to address this in a follow on refactor patch, since this one is already getting large in scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71125