[PATCH] D71144: [AIX] Use csect reference for function address constants

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1767
+            TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GO);
+        GOSym->setStorageClass(SC);
+        MCSectionXCOFF *Csect = OutStreamer->getContext().getXCOFFSection(
----------------
DiggerLin wrote:
> DiggerLin wrote:
> > hubert.reinterpretcast wrote:
> > > DiggerLin wrote:
> > > > Xiangling_L wrote:
> > > > > Since this is the case where one MCSectionXCOFF has only one MCSymbolXCOFF, when we want to get `SC`, we can do `MCSymbolXCOFF->getContainingCsect()`? (Please correct me if I am wrong) If so, do we still want to bother to set `SC` for MCSymbolXCOFF in this case?
> > > > we will output GOSym into symboltable in the XCOFFObjectWriter::writeSymbolTableEntryForCsectMemberLabel()
> > > > it get the storageclass directly 
> > > >  W.write<uint8_t>(SymbolRef.getStorageClass()); 
> > > Thanks for point this out, @DiggerLin. It sounds like this change may introduce a difference how the symbol table entries for the csect and the label are produced. Please look into adding appropriate testing. Note that the status quo in `llvm/test/CodeGen/PowerPC/aix-func-dsc-gen.ll` does not use the same storage class for the csect and the label. Note also that it is not generally okay to use the same storage class for the csect and the label (https://reviews.llvm.org/D71125?id=232583#inline-646944).
> > > 
> > > I believe @daltenty can elaborate on the refactor he mentions (https://reviews.llvm.org/D71144?id=232631#inline-644323). If I understand correctly, said refactor looks to be unavoidable. If this patch is indeed stuck, please indicate it. I think the "Plan Changes" option in Phabricator is a good way to do so.
> > the patch is only for the assembly,  not for the xcoff objectfile output.
> > 
> > because we do not invoke a getAssembler().registerSection(*Section) and 
> > getAssembler().registerSymbol(*Symbol)  in the  patch.
> > it can not output the  foo (extern function) symbol entry in the symbol table in xcoff objectfile.
> > and  it also can not output the  csect in the symbol table(it is not necessary need csect symbol output here ).
> > 
> > I will create a new patch for to output the  foo (extern function) symbol entry in the symbol table in xcoff objectfile later.
> > 
> > And I will change the storage class for SC to HIDDEN  here.
> > 
> >   
> sorry , change the storage class from SC to HIDDEN will have problem.  since we look external symbol as csect. and we will use  writeSymbolTableEntryForControlSection to write the external function symbol
@DiggerLin 
I don't think we need to generate the GOSym (now the FSym) in the symbol table. 
Currently in the XCOFFObjectWriter.cpp, if it's an undefined symbol, we would get the symbol's containingCsect, and generate the csect as symbol. If it's not an undefined symbol, we are referring the csect symbol right now, we are still generating the csect as symbol. We do not have label symbol involved for reference for function address.

@hubert.reinterpretcast @daltenty 
Could you elaborate on why you think the refactoring is unavoidable? I'm a bit confusing here. An example would be great (if possible).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71144