[PATCH] D75866: [AIX] supporting the visibility attribute for aix assembly

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/MC/MCAsmInfo.h:98
+  /// .global, .weak, .extern ,.comm.   Default is true.
+  bool HasVisibilityDirective = true;
+
----------------
Suggestion:
`HasVisibilityOnlyWithLinkage = false`


================
Comment at: llvm/include/llvm/MC/MCStreamer.h:569
 
+  virtual void EmitXCOFFSymbolAttrWithVisibility(MCSymbol *Symbol,
+                                                 MCSymbolAttr Attribute,
----------------
Are all the attributes emitted "with visibility" linkage attributes? If so, maybe `EmitXCOFFSymbolLinkageWithVisibility`?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1431
+    else
+      EmitLinkage(&F, Name);
   }
----------------
Given just the meaning associated with the naming of "hasVisibilityDirective", why is calling `EmitLinkage` the correct alternative?


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:832
+  case MCSA_Hidden:
+    OS << " , hidden";
+    break;
----------------
Why the extra space? Also below.


================
Comment at: llvm/lib/MC/MCStreamer.cpp:1076
+  llvm_unreachable(
+      "The EmitXCOFFSymbolAttrWithVisibility only supported on XCOFF targets");
+}
----------------
Insert "is" before "only".


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:48
+void MCXCOFFStreamer::EmitXCOFFSymbolAttrWithVisibility(
+    MCSymbol *Symbol, MCSymbolAttr Attribute, MCSymbolAttr Visibilitily) {
+  EmitSymbolAttribute(Symbol, Attribute);
----------------
Whole file:
s/Visibilitily/Visibility/g;



================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:50
+  EmitSymbolAttribute(Symbol, Attribute);
+  // Fixed me: Not implement Visibility on the object file.
+  // EmitSymbolAttribute(Symbol,Visibilitily);
----------------
s/Fixed me/FIXME/;
s/Not implement/Have not implemented/;


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:51
+  // Fixed me: Not implement Visibility on the object file.
+  // EmitSymbolAttribute(Symbol,Visibilitily);
+}
----------------
Silently incorrect output is not desirable. Uncomment this and ensure that we get a sensible failure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75866