[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 2 inline comments as done.
jasonliu added inline comments.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:865
+  // is one.
+  if (Symbol->isXCOFF() && cast<MCSymbolXCOFF>(Symbol)->hasRename())
+    emitXCOFFRenameDirective(Symbol,
----------------
DiggerLin wrote:
> I do not think print out rename directive here is  a good idead,  as the function name :emitCommonSymbol suggestion , it just print the .comm directive, and it is common function for all the target(not only for XCOFF).
> 
> I think it maybe better to put the code in the function emitGlobalVariable() before we call the  OutStreamer->emitCommonSymbol(GVSym, Size, Align);
> 
As I mentioned in the other comment, emitGlobalVariable() gets called on both assembly path and object file generation path as well. So we could not directly call emitXCOFFRenameDirective() there because we only wants to do it on the assembly generation path.
I understand it is common function for all the target, but it doesn't mean we could not do target specific staff in this function (See all the MAI calls).
I would argue for XCOFF specifically, emitCommonSymbol would need to tight together with this .rename everywhere (meaning if you need to emit a .comm directive, you need to check if the symbol used have a rename, if so emit .rename), so it kinda of make sense to do it here, as oppose to making sure every future use of emitCommonSymbol call on the assembly path accompany by a emitRename call below.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1610
+           "InternalLinkage should not have other visibility setting.");
+    LinkageAttr = MCSA_LGlobal;
+    break;
----------------
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).


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

https://reviews.llvm.org/D82481