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

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/MC/MCSymbolXCOFF.h:64
+  StringRef getSymbolTableName() const {
+    if (!SymbolTableNameEntry.empty())
+      return SymbolTableNameEntry;
----------------
 even if  there is some Overhead, using hasRename() can let user know what the getSymbolTableName() do ? 




================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:865
+  // is one.
+  if (Symbol->isXCOFF() && cast<MCSymbolXCOFF>(Symbol)->hasRename())
+    emitXCOFFRenameDirective(Symbol,
----------------
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);



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


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

https://reviews.llvm.org/D82481