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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/MC/MCAsmInfoXCOFF.cpp:48
+  // any combination of these.
+  return (C >= 'a' && C <= 'z') || (C >= 'A' && C <= 'Z') ||
+         (C >= '0' && C <= '9') || C == '_' || C == '.';
----------------
Please call `isAlnum`.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:803
     MCSymbol *Symbol, MCSymbolAttr Linkage, MCSymbolAttr Visibility) {
+  // Print symbol's rename(original name contains invalid character(s)) if there
+  // is one.
----------------
See other comment about missing space before left parenthesis.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:848
+  Name->print(OS, MAI);
+  OS << ',' << '"' << Rename << '"';
+  EmitEOL();
----------------
To escape double quote (`"`) characters, the character should be doubled. Unlike with names that start with digits, Clang does manage to handle quotes in names.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:863
                                      unsigned ByteAlignment) {
+  // Print symbol's rename(original name contains invalid character(s)) if there
+  // is one.
----------------
Minor nit: The space is missing before the first opening parenthesis.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:865
+  // is one.
+  if (Symbol->isXCOFF() && cast<MCSymbolXCOFF>(Symbol)->hasRename())
+    emitXCOFFRenameDirective(Symbol,
----------------
We're performing the same cast/type check multiple times here...

Suggestion:
```
  MCSymbolXCOFF *XSym = dyn_cast<MCSymbolXCOFF>(Symbol);
  if (XSym && XSym->hasRename()) {
```


================
Comment at: llvm/lib/MC/MCContext.cpp:317
+  // as prefix to signal this is an renamed symbol.
+  const bool IsEntryPoint = InvalidName[0] == '.';
+  SmallString<128> ValidName =
----------------
Don't try to access the first character without locally ensuring that the string is non-empty. For example, ICC produces an "internal error" (https://godbolt.org/z/zLiJqb). It seems Clang avoids generating such by diagnosing empty string literals used as `__asm__` names.


================
Comment at: llvm/lib/MC/MCContext.cpp:321
+
+  // Append the hex values of '_' and invalid characters with "_Renamed..",
+  // at the same time replace invalid characters with '_'.
----------------
Replace the comma at the end of the line with a semicolon.


================
Comment at: llvm/lib/MC/MCStreamer.cpp:1076
+                                          StringRef Rename) {
+  llvm_unreachable("emitXCOFFRenameDirecitve is only supported on "
+                   "XCOFF targets");
----------------
s/emitXCOFFRenameDirecitve/emitXCOFFRenameDirective/;


================
Comment at: llvm/lib/MC/MCSymbolXCOFF.cpp:22
+         "SymbolTableNames need to be the same for this symbol and its csect "
+         "representatation.");
   return RepresentedCsect;
----------------
s/representatation/representation/;


================
Comment at: llvm/lib/MC/MCSymbolXCOFF.cpp:37
+         "SymbolTableNames need to be the same for this symbol and its csect "
+         "representatation.");
   RepresentedCsect = C;
----------------
s/representatation/representation/;


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp:123
   void emitTCEntry(const MCSymbol &S) override {
-    const MCAsmInfo *MAI = Streamer.getContext().getAsmInfo();
-    OS << "\t.tc ";
-    OS << (MAI->getSymbolsHaveSMC()
-               ? cast<MCSymbolXCOFF>(S).getUnqualifiedName()
-               : S.getName());
-    OS << "[TC],";
-    OS << S.getName();
-    OS << '\n';
+    if (S.isXCOFF()) {
+      MCSymbolXCOFF *TCSym =
----------------
```
if (MCSymbolXCOFF *XSym = dyn_cast<MCSymbolXCOFF>(S)) {
```


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-symbol-rename.ll:1
+;; This file tests how llc handles symbol contains invalid character on XCOFF
+;; platform.
----------------
s/symbol contains/symbols containing/;
s/invalid character/invalid characters/;


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-symbol-rename.ll:2
+;; This file tests how llc handles symbol contains invalid character on XCOFF
+;; platform.
+;; Since symbol name resolution is the same between 32 bit and 64 bit,
----------------
s/XCOFF platform/an XCOFF platform/;


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-symbol-rename.ll:3
+;; platform.
+;; Since symbol name resolution is the same between 32 bit and 64 bit,
+;; test for 64 bit mode is omitted.
----------------
s/32 bit/32-bit/;
s/64 bit/64-bit/;


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-symbol-rename.ll:4
+;; Since symbol name resolution is the same between 32 bit and 64 bit,
+;; test for 64 bit mode is omitted.
+
----------------
s/test/tests/;
s/64 bit/64-bit/;
s/is/are/;


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

https://reviews.llvm.org/D82481