[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:96
 
+  /// True if this is a XCOFF target that supports visibility attribute in
+  /// .global, .weak, .extern ,.comm.   Default is false.
----------------
Suggestion (please copy verbatim):
```
  /// True if this is an XCOFF target that supports visibility attributes as
  /// part of .global, .weak, .extern, and .comm. Default is false.
```


================
Comment at: llvm/include/llvm/MC/MCStreamer.h:570
+  virtual void EmitXCOFFSymbolLinkageWithVisibility(MCSymbol *Symbol,
+                                                    MCSymbolAttr Attribute,
+                                                    MCSymbolAttr Visibilitily);
----------------
Rename `Attribute` to `Linkage` to reflect the intended meaning. Please do so for all instances of this function and its overriders.


================
Comment at: llvm/include/llvm/MC/MCStreamer.h:572
+                                                    MCSymbolAttr Visibilitily);
   /// Emit an ELF .size directive.
   ///
----------------
Add a blank line before this comment block.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:811
+  default:
+    EmitSymbolAttribute(Symbol, Attribute);
+    return;
----------------
Why is this okay? The visibility would get silently ignored. This should be an `llvm_unreachable` and caller should avoid calling this function in such a way that this case is hit.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:828
+
+  switch (Visibilitily) {
+  default:
----------------
Why no `MCSA_Internal` case?


================
Comment at: llvm/lib/MC/MCStreamer.cpp:1075
+  llvm_unreachable(
+      "The EmitXCOFFSymbolLinkageWithVisibility is only supported on "
+      "XCOFF targets");
----------------
s/The //;
Also, add a period at the end of the sentence.


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:51
+
+  if (Visibility == MCSA_Invalid)
+    return;
----------------
There should be a comment here that `MCSA_Invalid` is used to indicate that there is no visibility to emit. There should also be a Doxygen comment in the base interface declaration that indicates this aspect of the function interface.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1590
+
+  switch (Visibiltiy) {
+  default:
----------------
This needs a TODO for implementing "exported" versus "unspecified" visibility for AIX.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll:44
+; CHECK:        .globl  b_h, hidden
+; CHECK:        .extern .bar_h, hidden
----------------
There is no testing for referencing the address of a function with hidden visibility that is neither defined nor called directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75866