[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/MCXCOFFStreamer.h:29
   void EmitInstToData(const MCInst &Inst, const MCSubtargetInfo &) override;
+  void emitXCOFFSymbolLinkageWithVisibility(MCSymbol *Symbol,
+                                            MCSymbolAttr Linkage,
----------------
Minor nit: Please declare this in the same order as the base class in relation to the next declaration.


================
Comment at: llvm/lib/MC/MCAsmInfoXCOFF.cpp:19
   COMMDirectiveAlignmentIsInBytes = false;
+  HasVisibilityOnlyWithLinkage = true;
   LCOMMDirectiveAlignmentType = LCOMM::Log2Alignment;
----------------
Minor nit: Move this to right after `IsLittleEndian` to match the ordering in `MCAsmInfo`.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:807
+    break;
+  case MCSA_Weak:
+    OS << MAI->getWeakDirective();
----------------
The calls to a function with this name never pass `MCSA_Weak`. The case makes sense to have but we need some way to get here.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:814
+  default:
+    llvm_unreachable("Visibility is not a part of this linkage type.");
+    return;
----------------
I think the message should just say "Unhandled linkage type".


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:833
+  default:
+    break;
+  case MCSA_Hidden:
----------------
This should be the case for `MCSA_Invalid`, but for other values, we should call `llvm_unreachable` indicating that we got an unexpected value for `Visibility`.


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


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:57
+
+  ///< Not emit default visibility.
+  if (Visibility == MCSA_Invalid)
----------------
The comment here should not be Doxygen style. There should be a comment elsewhere (in the header for the base class declaration) that //is// in Doxygen style. Also, "default visibility" is ambiguous for XCOFF.

Suggestion:
```
  // When the caller passes `MCSA_Invalid` for the visibility, do not emit one.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1549
+  switch (GV->getVisibility()) {
+   case  GlobalValue::DefaultVisibility:
+    break;
----------------
Fix the indentation. Also, the TODO for the "exported" versus "unspecified" needs to go here.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1551
+    break;
+  
+  case GlobalValue::HiddenVisibility:
----------------
Be consistent about the use of blank lines between cases.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1570
+  default:
+    //TODO:Need to deal with CommonLinkage, CommonLinkage etc. 
+    AsmPrinter::emitLinkage(GV, GVSym);
----------------
Please add spaces:
```
// TODO: Need to [ ... ]
```

Please add a comma before "etc.".

`CommonLinkage` is listed twice?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1571
+    //TODO:Need to deal with CommonLinkage, CommonLinkage etc. 
+    AsmPrinter::emitLinkage(GV, GVSym);
+    return;
----------------
This should not just ignore the visibility. `report_fatal_error` if we encounter this case and the `VisibilityAttr` is not `MCSA_Invalid`.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1603
+  default:  
+    AsmPrinter::EmitLinkage(GV, GVSym);
+    return;
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > hubert.reinterpretcast wrote:
> > > Why is it okay to ignore visibility when encountering this case?
> > in the patach, we only deal with the GlobalValue::ExternalLinkage
> > for other linkage , we do not deal with in the patch.
> >   case GlobalValue::CommonLinkage:
> >   case GlobalValue::LinkOnceAnyLinkage:
> >   case GlobalValue::LinkOnceODRLinkage:
> >   case GlobalValue::WeakAnyLinkage:
> >   case GlobalValue::WeakODRLinkage:
> > 
> >   we do not deal with this moment, we call the  AsmPrinter::emitLinkage
> > 
> > we maybe need to deal above linkage later in other patch.
> Also, please use the immediate base class when explicitly invoking a base implementation of a virtual function.
This is not fixed. Please edit the call to `AsmPrinter::emitLinkage`.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1607
+    // If external, declare as a global symbol: .globl _foo
+    if (GV->isDeclaration())
+      OutStreamer->EmitXCOFFSymbolLinkageWithVisibility(GVSym, MCSA_Extern,
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > Would it make sense to do
> > `GV->isDeclaration() ? MCSA_Extern : MCSA_Global`
> > ?
> if the GV only is declare. we will emit .extern	Name [ , Visibility ]  here 
> if the GV is define . we will emit .globl	Name [, Visibility ]
Which is exactly what using the conditional expression would do also.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75866