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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/MC/MCAsmStreamer.cpp:821
+    if (Symbol->getName() == QualName->getUnqualifiedName() &&
+        XCOFFSym->getContainingCsect()->getMappingClass() != XCOFF::XMC_PR)
+      QualName->print(OS, MAI);
----------------
hubert.reinterpretcast wrote:
> I am somewhat surprised to encounter this block of code as being something new in the codebase since the problem it solves should not be a "new problem".
> 
> If I understand this correctly, this block of code solves a problem that this patch does need a solution for but is not the subject of this patch. In particular, we are still (without this patch) not emitting the linkage on the assembly path for references to functions that are not defined in the current translation unit. I would suggest splitting this out into a separate patch first. @jasonliu @daltenty, fya.
Yes, we do not emit .extern for undefined function. And that was fine because we used -u option with assembler to work around that.
And when I look into this change here, I realized that we are changing the architect of how we are emitting the assembly and it definitely deserve a separate patch and we should discuss before we proceed.
Before, we would generate:
 .globl  foo
with this patch:
 .globl  foo[DS]

This means we are changing what we are seeing in the symbol table.
Before, we would have 1 csect named foo with internal linkage, and 1 label named foo with external linkage.
Now, we have 1 csect named foo with external inkage. And that's it. But we still emit the useless foo label in the assembly though.
So IMO, there are some clean up we might want to do before we proceed with this ".globl foo[DS]" route, those clean up including: start generating .globl for function descriptor csect, and stop generating the useless function descriptor label, generate .extern for functions and variables, get rid of '-u' option. There are test cases to clean up as well, all those tests checking ".globl foo", we need to change them to check ".globl foo[DS]". Those tests could still be passing now because they are still "match", but we should check what we actually want to test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75866