[PATCH] D82481: [XCOFF][AIX] Give symbol an internal name when desired symbol name contains invalid character(s)

[Jason Liu via Phabricator via llvm-commits]

jasonliu marked an inline comment as done.
jasonliu added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1610
+           "InternalLinkage should not have other visibility setting.");
+    LinkageAttr = MCSA_LGlobal;
+    break;
----------------
DiggerLin wrote:
> jasonliu wrote:
> > DiggerLin wrote:
> > > I think it maybe better to call emitSymbolAttribute(GVSym, MCSA_LGlobal)  and then return directly,
> > > 
> > > Reason: 
> > > 1. for the .lglobl do not have have visibility, it do not need to go to emitXCOFFSymbolLinkageWithVisibility() ,
> > > 2. we can move the code  
> > >    if (cast<MCSymbolXCOFF>(Symbol)->hasRename())
> > >     OutStreamer->emitXCOFFRenameDirective(Symbol,
> > >                              cast<MCSymbolXCOFF>(Symbol)->getSymbolTableName());
> > > 
> > > into this function.
> > > 
> > > 3.  if you call  emitSymbolAttribute directly, we do not need to modify the function  emitXCOFFSymbolLinkageWithVisibility() in this patch.
> > We only want to emitXCOFFRenameDirective when it's emitting assembly though.
> > And emitLinkage is called on both assembly generating path and object generation path, so we can't move that call directly to here.
> > It's still possible to make it happen by wrapping emitXCOFFRenameDirective with another new function(something like handleXCOFFRename?), and have that function do emitXCOFFRenameDirective on the assembly generation side, but do nothing on the object file generation side.
> > But I don't find adding that new function to be particular appealing though (we need to add so many extra boilerplate to tell other target that this function is not meant for them, we already did it for emitXCOFFRenameDirective but that's necessary evil because that directive is indeed not supported by other target).
> If we change the function 
> ```
> void MCStreamer::emitXCOFFRenameDirective(const MCSymbol *Name,
>                                           StringRef Rename) {
>   llvm_unreachable("emitXCOFFRenameDirecitve is only supported on "
>                    "XCOFF targets");
> }
> ```
> to 
> ```
> void MCStreamer::emitXCOFFRenameDirective(const MCSymbol *Name,
>                                           StringRef Rename) {
> 
> }
> ```
> and object generation  path will do nothing for the emitXCOFFRenameDirective(), it just some overhead for object generation  path but the logic will be  more clear.
Object generation path will take the one in MCXCOFFStreamer.h, which currently doing report_fatal_error.
I'm doing report_fatal_error because we do not need to invoke that function for now. That function will need to set the symbol table name for a rename when we implement the AsmParser for XCOFF and it could not be an empty function that does nothing for that purpose. 


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

https://reviews.llvm.org/D82481