[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(
----------------
hubert.reinterpretcast wrote:
> jasonliu wrote:
> > 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).
> > I don't think we need to generate the GOSym (now the FSym) in the symbol table.
> @jasonliu, do you mean that we should stop generating a label for the function descriptor in both the assembly writing and the direct object file writing paths?
> 
> > sorry , change the storage class from SC to HIDDEN will have problem. since we look external symbol as csect
> @DiggerLin, unless if this code is changing, the csect storage class is an issue. A small solution might be as simple as checking `F->isDeclaration()`.
> 
> re: "the refactor":
> A proliferation of code to set symbol properties for the csect of function descriptors seems like a sign of potential timing problems (in addition to the usual problems that come with duplicated logic).
> 
> @jasonliu, do you mean that we should stop generating a label for the function descriptor in both the assembly writing and the direct object file writing paths?
We could choose not generating a label for function descriptor if we are sure that we are not going to reference the label anywhere and always use qualname. 
But what I meant back there is that if we do this: 

```
void foo() ;
void (*foo_ptr)() = &foo;
```
We do not need to generate the label for declaration foo(). (Yes, we will have the label if foo() is defined.) 

Yes, we could use the F->isDeclaration() to determine if the csect is C_EXT or HIDDEN for now. 

Thanks for the clarification of "the refactor". I don't think it's unavoidable, but definitely more than nice to have (for the timing problem).  



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71144