[AIX] supporting the visibility attribute for aix assembly

[DiggerLin]

Summary

1. in the aix assembly , it do not have .hidden and .protected directive.
2. in current llvm. if a function or a variable which has visibility attribute, it will generate something like the .hidden or .protected , it can not recognize by aix as.
3. in aix assembly, the visibility attribute are support in the pseudo-op like

.extern Name [ , Visibility ]
.globl Name [, Visibility ]
.weak Name [, Visibility ]

in this patch, we implement the visibility attribute for the global variable, function or extern function .

for example.

extern __attribute__ ((visibility ("hidden"))) int
  bar(int* ip);
__attribute__ ((visibility ("hidden"))) int b = 0;
__attribute__ ((visibility ("hidden"))) int
  foo(int* ip){
   return (*ip)++;
}

4. the visibility of .comm linkage do not support , we will have a separate patch for it.
5. we have the unsupported cases ("default" and "internal") , we will implement them in a a separate patch for it.

[hubert.reinterpretcast]

llvm/include/llvm/MC/MCAsmInfo.h

98

Suggestion:
HasVisibilityOnlyWithLinkage = false

llvm/include/llvm/MC/MCStreamer.h

568

Are all the attributes emitted "with visibility" linkage attributes? If so, maybe EmitXCOFFSymbolLinkageWithVisibility?

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1542

Given just the meaning associated with the naming of "hasVisibilityDirective", why is calling EmitLinkage the correct alternative?

llvm/lib/MC/MCAsmStreamer.cpp

833

Why the extra space? Also below.

llvm/lib/MC/MCXCOFFStreamer.cpp

64

Whole file:
s/Visibilitily/Visibility/g;

67

Silently incorrect output is not desirable. Uncomment this and ensure that we get a sensible failure.

[DiggerLin]

address comment

llvm/include/llvm/MC/MCAsmInfo.h

98

change as suggestion

llvm/include/llvm/MC/MCStreamer.h

568

not all attribute emitted with "with visibility" . changed as suggestion.

llvm/lib/MC/MCXCOFFStreamer.cpp

67

OK. I uncomment the EmitSymbolAttribute(Symbol,Visibility);
it will hit report_fatal_error("Not implemented yet.");

[jasonliu]

This patch does not apply to master, and it does not have dependency listed.
We should rebase this patch on master, or find out the right dependencies so that anyone could apply this patch and test.

[daltenty]

Are we forgetting the case of commons? They also have visibility as part of the assembler directive on AIX.

llvm/lib/MC/MCXCOFFStreamer.cpp

64

Not just this file, spelling issue is pervasive. Also check 'Visibiltiy'

[hubert.reinterpretcast]

llvm/include/llvm/MC/MCAsmInfo.h

96

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.

llvm/include/llvm/MC/MCStreamer.h

569

Rename Attribute to Linkage to reflect the intended meaning. Please do so for all instances of this function and its overriders.

577

Add a blank line before this comment block.

llvm/lib/MC/MCAsmStreamer.cpp

812

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.

829

Why no MCSA_Internal case?

llvm/lib/MC/MCStreamer.cpp

1069

s/The //;
Also, add a period at the end of the sentence.

llvm/lib/MC/MCXCOFFStreamer.cpp

67

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.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1588

This needs a TODO for implementing "exported" versus "unspecified" visibility for AIX.

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll

44 ↗

(On Diff #249487)

There is no testing for referencing the address of a function with hidden visibility that is neither defined nor called directly.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1585

Linkage and "Visibiltiy" are used only once. I would suggest that we don't need these variables.

1590

There should be a limited number of VisibilityTypes, make this a covering switch.

1601

Why is it okay to ignore visibility when encountering this case?

1604

The comment does not seem to help here, especially since the code has further conditions on the form of the output.

1618

This unreachable is "physically" unreachable...

[hubert.reinterpretcast]

llvm/lib/MC/MCAsmStreamer.cpp

841

This return is redundant with reaching the end of the function.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1605

Would it make sense to do
GV->isDeclaration() ? MCSA_Extern : MCSA_Global
?

1613

For AIX asm, I would like to believe that this is always true.

[hubert.reinterpretcast]

llvm/lib/MC/MCAsmStreamer.cpp

822

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.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1601

Also, please use the immediate base class when explicitly invoking a base implementation of a virtual function.

[DiggerLin]

address comment

llvm/lib/MC/MCAsmStreamer.cpp

829

not sure when the llvm will emit a internal visibility.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1590

yes, there are only three visibility

/// An enumeration for the kinds of visibility of global values.
enum VisibilityTypes {
  DefaultVisibility = 0,  ///< The GV is visible
  HiddenVisibility,       ///< The GV is hidden
  ProtectedVisibility     ///< The GV is protected
};

1601

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.

1605

if the GV only is declare. we will emit .extern Name [ , Visibility ] here
if the GV is define . we will emit .globl Name [, Visibility ]

[jasonliu]

llvm/lib/MC/MCAsmStreamer.cpp

822

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.

[hubert.reinterpretcast]

llvm/lib/MC/MCAsmStreamer.cpp

822

Emitting .weak (when that is needed) is necessary and fits with emitting .extern.

[hubert.reinterpretcast]

llvm/include/llvm/MC/MCXCOFFStreamer.h

29

Minor nit: Please declare this in the same order as the base class in relation to the next declaration.

llvm/lib/MC/MCAsmInfoXCOFF.cpp

17

Minor nit: Move this to right after IsLittleEndian to match the ordering in MCAsmInfo.

llvm/lib/MC/MCAsmStreamer.cpp

805

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.

812

I think the message should just say "Unhandled linkage type".

831

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.

llvm/lib/MC/MCStreamer.cpp

1069

There is still no period at the end of the sentence.

llvm/lib/MC/MCXCOFFStreamer.cpp

68

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.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1587

Fix the indentation. Also, the TODO for the "exported" versus "unspecified" needs to go here.

1589

Be consistent about the use of blank lines between cases.

1601

This is not fixed. Please edit the call to AsmPrinter::emitLinkage.

1605

Which is exactly what using the conditional expression would do also.

1608

Please add spaces:

// TODO: Need to [ ... ]

Please add a comma before "etc.".

CommonLinkage is listed twice?

1609

This should not just ignore the visibility. report_fatal_error if we encounter this case and the VisibilityAttr is not MCSA_Invalid.

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll

44 ↗

(On Diff #249487)

Please add the requested test.

[DiggerLin]

We(David, Hubert, Jason, Sean and me) has discussed about the visibility this afternoon.

1. We agree to separate into three patch.

First patch:

cat > test.c
void foo() {};

Currently, llc will generate assembly code as (assembly patch)

.globl  foo
.globl  .foo
.csect foo[DS]

foo:

     .long   .foo
     .long   TOC[TC0]
     .long   0
and symbol table as (xcoff object file)
[4]     m   0x00000004     .data     1  unamex                    foo
[5]     a4  0x0000000c       0    0     SD       DS    0    0
[6]     m   0x00000004     .data     1  extern                    foo
[7]     a4  0x00000004       0    0     LD       DS    0    0
After first patch, the assembly will be as
     .globl  foo[DS]                 # -- Begin function foo
     .globl  .foo
     .align  2
     .csect foo[DS]
     .long   .foo
     .long   TOC[TC0]
     .long   0
 and symbol table will as
[6]     m   0x00000004     .data     1  extern                    foo
[7]     a4  0x00000004       0    0     DS      DS    0    0

we need to change the code for the assembly path and xcoff object file patch for llc.
and also need to modify all the aix test cases which relate to above change
Second Patch.

We will support .extern and .weak directive in Second Patch.

Third Patch

We will support visibility in third patch. (this patch will be the third patch)

[DiggerLin]

llvm/lib/MC/MCAsmStreamer.cpp

831

I do not think the MCSA_Invalid will come to here. if come to the function. it should be llvm_unreachable too.

[hubert.reinterpretcast]

llvm/lib/MC/MCAsmStreamer.cpp

831

PPCAIXAsmPrinter::emitLinkage uses MCSA_Invalid for GlobalValue::DefaultVisibility.

[DiggerLin]

change code based on the emit .extern and .weak patch

[jasonliu]

llvm/include/llvm/MC/MCSymbolXCOFF.h

63

This makes every symbol by default is internal visibility?
On ELF, the default visibility for symbol is default. On AIX, if visibility feature is turned on, the default is unspecified.
Setting it to internal here might not be what we want. There is visibility internal. Try to implement visibility unspecified with internal is confusing.

llvm/lib/MC/MCAsmStreamer.cpp

823

Is there a place we passed in the wrong symbol?
I would expect we could do symbol printing directly instead of querying if a qualname need to be emitted.

833

We don't like spaces between commas: https://reviews.llvm.org/D80247

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll

1 ↗

(On Diff #262418)

Missing mcpu settings (the default mcpu is not correct for AIX).

56 ↗

(On Diff #262418)

I'm only seeing hidden visibility attr and unspecified being test here.
But we could have 5 combination here with globl directive. I would like to list here so we could be clear:

1. no visibility attribute -> unspecified
2. exported -> default
3. hidden -> hidden
4. internal -> internal
5. protected -> protected

It's not clear from the patch description, how many of this visibility attribute we are going to support. If we don't say anything, I would assume we are going to support all of them. Then we are clearly missing some implementation and testing here.

[DiggerLin]

llvm/include/llvm/MC/MCSymbolXCOFF.h

63

thanks ,changed to XCOFF::SYM_V_UNSPECIFIED

63

thanks for your suggestion.

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll

1 ↗

(On Diff #262418)

thanks

56 ↗

(On Diff #262418)

I tried the

__attribute__ ((visibility ("default"))) void foo_default(int *p){
  (*p)++;
}

__attribute__ ((visibility ("internal"))) void foo_internal(int *p){
  (*p)++;
} 

with clang it convert to  
define void @foo_default(i32* %p) #0 {
entry:
  %p.addr = alloca i32*, align 4
  store i32* %p, i32** %p.addr, align 4
  %0 = load i32*, i32** %p.addr, align 4
  %1 = load i32, i32* %0, align 4
  %inc = add nsw i32 %1, 1
  store i32 %inc, i32* %0, align 4
  ret void
}

define hidden void @foo_internal(i32* %p) #0 {
entry:
  %p.addr = alloca i32*, align 4
  store i32* %p, i32** %p.addr, align 4
  %0 = load i32*, i32** %p.addr, align 4
  %1 = load i32, i32* %0, align 4
  %inc = add nsw i32 %1, 1
  store i32 %inc, i32* %0, align 4
  ret void
}

clang look "default" visibility as no visibility, and look "internal" as "hidden"

static Visibility parseVisibility(Arg *arg, ArgList &args,
                                 DiagnosticsEngine &diags) {
 StringRef value = arg->getValue();
 if (value == "default") {
   return DefaultVisibility;
 } else if (value == "hidden" || value == "internal") {
   return HiddenVisibility;
 } else if (value == "protected") {
   // FIXME: diagnose if target does not support protected visibility
   return ProtectedVisibility;
 }

diags.Report(diag::err_drv_invalid_value)
  << arg->getAsString(args) << value;
return DefaultVisibility;

}

I think we need a separate patch for "default" and "hidden"

56 ↗

(On Diff #262418)

added a protected test case

[DiggerLin]

address comment

[jasonliu]

llvm/include/llvm/BinaryFormat/XCOFF.h

181

I feel like "Symbol" here is redundant. We could just name it VisibilityType, No?

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1528

if (TM.getTargetTriple().isOSBinFormatXCOFF() && !F.isIntrinsic()) {
  ...
  emitLinkage()
} else {
  ...
  emitVisibility()
}

Might be a better choice than "continue" here. If F is an intrinsic then it would go to "emitVisibility" block even if it's XCOFF target. It may or may not do any harm, but I think it would be better not go to there.

llvm/lib/MC/MCAsmStreamer.cpp

812

I think the message should just say "Unhandled linkage type".

This comment is not addressed.

Should this really be llvm_unreachable? It's possible someone passed in a wrong value by mistake, and we would still like to catch that in a non-assert build. Also I don't think we would have a good binary if we hit this. A report_fatal_error might be preferable.

813

nit: do we need this return?

816

Remove unused local variable: XCOFFSym

822

Should be report_fatal_error?

llvm/lib/MC/MCXCOFFStreamer.cpp

68

There should be a comment elsewhere (in the header for the base class declaration) that is in Doxygen style.

I'm not seeing this comment being addressed?

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1584

Do we want to assert here?
assert(MAI->HasVisibilityOnlyWithLinkage(), "AIX's linkage directive have visibility setting.");
which serves as a documentation as well.

1588

Right now, we already implemented "unspecified" visibility using GlobalValue::DefaultVisibility. So the comment is not correct.
I think we are missing "exported" and "internal" visibility.

1589

nit: remove extra blank line?

1615

What is the extra linkage we need to deal here?
If we know the extra linkage we need to deal with and we don't want to deal with them now, then we have separate cases for those extra linkages and call AsmPrinter::emitLinkage(GV, GVSym);.
Ideally, I would want the default case to be report_fatal_error so that if we miss handling something, it would trigger.

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll

56 ↗

(On Diff #262418)

Could we have the unsupported cases ("default" and "internal") documented in the patch summary(later in commit message)?
We should say this patch only implements visibility "unspecifeid", "protected" and "hidden" on AIX. "default" and "internal" will be handled in a separate patch because they would require IR level change in clang to support.

9 ↗

(On Diff #266213)

We are testing visibility here, do you really need all this code block inside "foo", "foo_h", "foo_protected", "foo_weak_h"?

[jasonliu]

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll

56 ↗

(On Diff #262418)

Also, we should note explicitly, .comm directory's visibility is not implemented?

[jasonliu]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1528

Sorry, I think this might be better.

if (TM.getTargetTriple().isOSBinFormatXCOFF())
  if (!F.isIntrinsic())
    continue;
  ...
  emitLinkage()
} else {
  ...
  emitVisibility()
}

[DiggerLin]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1615

I added AvailableExternallyLinkage . I think we have deal with all the linkage with visibility. others we just pass to the AsmPrinter::emitLinkage(GV, GVSym) and let AsmPrinter::emitLinkage deal with it.

[DiggerLin]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1615

sorry, please discard above comment.

[DiggerLin]

address comment

[jasonliu]

llvm/include/llvm/MC/MCSymbolXCOFF.h

58

const before { ?

llvm/lib/MC/MCAsmStreamer.cpp

31

how many of these newly added include directive are actually needed now?

799

Could we add const for the Symbol argument?

812

Should we do a report_fatal_error instead?

llvm/lib/MC/MCXCOFFStreamer.cpp

68

I don't think removing

if (Visibility == MCSA_Invalid)
    return;

before emit symbol attribute for visibility, and skip the requested comment is a good way to move forward.
Without those, there is no clear contract to tell people how to use this function.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1584

This is marked done, but I'm not seeing anything?

1588

'versus' does not sound right in the current context. It should be 'and'?

1591

We should call the direct derived class's method (even when that class did not override this function).

1603

I think it might be better to do assignments here just like the visibility above. And do only one call out to emitXCOFFSymbolLinkageWithVisibility outside of this switch statement. It would help the readability of this code block as it clearly showed that this switch only modify one argument.

1619

nit: commonLinkage -> CommonLinkage
The name is spelled with Capitalization, so we should stick with that.

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll

1 ↗

(On Diff #267360)

nit: a split for these line would be helpful? As it's a tad too long.

56 ↗

(On Diff #262418)

I'm not seeing this getting responded or addressed.

[DiggerLin]

llvm/lib/MC/MCAsmStreamer.cpp

31

thanks

799

sorry , I can not add const in this patch, for in the function
MCXCOFFStreamer::emitXCOFFSymbolLinkageWithVisibility()

the function calls the
bool MCXCOFFStreamer::emitSymbolAttribute(MCSymbol *Sym,Symbol, Visibility)

. if we want to change to " const MCSymbol *Sym" , we can add a NFC patch later.

llvm/lib/MC/MCXCOFFStreamer.cpp

68

if Visibility is MCSA_Invalid, it will not come to function emitXCOFFSymbolLinkageWithVisibility

[DiggerLin]

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll

56 ↗

(On Diff #262418)

added into the patch's summary

[DiggerLin]

address comment

[jasonliu]

llvm/lib/MC/MCAsmStreamer.cpp

799

You are right. I didn't realize it needs to be non-const for MCXCOFFStreamer version.

[jasonliu]

llvm/lib/MC/MCXCOFFStreamer.cpp

68

Thanks. I would actually want the emitXCOFFSymbolLinkageWithVisibility to handle MCSA_Invalid case. Please see my other comment.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1593

I have some concern about this call back to the original AsmPrinter::emitLinkage.
It seems this is just a short cut so that we don't need to handle the linkage emission with unspecified visibility.
But I would argue that taking this short cut might not worth it, as it's not too much work for the emitXCOFFSymbolLinkageWithVisibility to handle the unspecified visibility case.
If we don't need to call back to the base AsmPrinter::emitLinkage, then we could remove a lot of XCOFF-related staff out from AsmPrinter::emitLinkage which would make the code a lot cleaner. Also we could avoid dual-maintaining two emitLinkage method for AIX.

[DiggerLin]

llvm/lib/MC/MCXCOFFStreamer.cpp

68

please see my other comment.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1593

some linkage do not have visibility.
for example
InternalLinkage .lglobl , it should go to PPCAsmPrinter::emiltLinkage.

and there also have PrivateLinkage and AppendingLinkage, we do not deal with in the function , it just need to pass to function PPCAsmPrinter::emiltLinkage to deal with it.

I discuss with jason offline, Jason's option is "we should deal with all the linkage and visibility in current function without invoke the PPCAsmPrinter::emiltLinkage()" .
@hubert.reinterpretcast , what is your option ?

[DiggerLin]

address comment

[jasonliu]

LGTM with minor nit.

llvm/include/llvm/MC/MCStreamer.h

572

nit, we need to mention it here as Doxygen style:
When the symbol do not have any visibility setting, pass in MCSA_Invalid.

llvm/lib/MC/MCAsmStreamer.cpp

818

nit: we should make the default case position more consistent at least within this function/file.

819

nit: Visibility -> visibility type

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1593

Thanks for the change. I do like what we have right now because we don't need to worry if the base emitLinkage have the correct implementation for AIX or not.
Are we planning to clean up the base emitLinkage (remove XCOFF-related changes) in a separate NFC patch?

[hubert.reinterpretcast]

llvm/include/llvm/BinaryFormat/XCOFF.h

181

A lot of the comments in the file should be Doxygen-style, but the relative dearth of them is no reason not to use one here.

We should have a comment:
Values for visibility as they would appear when encoded in the high 4 bits of the 16-bit unsigned n_type field of symbol table entries. Valid for 32-bit XCOFF only when the vstamp in the auxiliary header is greater than 1.

llvm/include/llvm/MC/MCStreamer.h

568

s/in linkage directive/with a linkage directive/;

572

Suggestion:
... to emit or MCSA_Invalid is the symbol does not have an explicit visibility.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1520–1521

I would suggest flipping this:

if (!TM.getTargetTriple().isOSBinFormatXCOFF()) {
  // ...
  continue;
}

if (F.isIntrinsic())
  continue;

// ...

llvm/lib/MC/MCAsmInfoXCOFF.cpp

17

This should be rebased for new changes.

llvm/lib/MC/MCAsmStreamer.cpp

821

Should there be a comment like

// Nothing to do.

?

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1586

s/directive have visibility/directives take a visibility/;

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll

1 ↗

(On Diff #268485)

Please fix the two consecutive spaces before -mtriple.

3 ↗

(On Diff #268485)

Same comment.

9 ↗

(On Diff #268485)

Fix the two spaces before the closing brace.

14 ↗

(On Diff #268485)

Fix the two spaces after define.

19 ↗

(On Diff #268485)

Same comment.

43 ↗

(On Diff #268485)

This does not actually prevent ,something from being on this line.
Add {{[[:space:]]*([#].*)?$}}.

44 ↗

(On Diff #268485)

Same comment.

47 ↗

(On Diff #268485)

The file is called aix-xcoff-hidden.ll? Should it be renamed?

54 ↗

(On Diff #268485)

Does it make sense to have a blank line between the defined variables and the undefined symbols?

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1606

Should there be an assert for MAI->hasDotLGloblDirective()?

1609

llvm_unreachable("Should never emit this");

is right. This linkage type is an LLVM IR artifact.

1611

I believe that llvm_unreachable is generally correct for cases where the source of the error condition is not from "user input" but instead from API usage error.

1612

Please assert here that LinkageAttr is not still MCSA_Invalid.

[DiggerLin]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1593

Only one place need to clean in the base emitLinkage, I clean it in this patch.

llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll

47 ↗

(On Diff #268485)

thanks

[DiggerLin]

rebase the patch and address comment

[jasonliu]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

424

hasDotExternDirective is XCOFF only.
Also, please check if we have other usage of this MAI property. If not, we could remove them together.

432

XCOFF only. Clean up needed. Please also check if we could remove this property from MAI.

436

This is XCOFF only, which could get clean up.

442

XCOFF only. Clean up needed.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1593

There is more than one place. Please check my other comments if we want to clean it in this patch.

[DiggerLin]

in order not to let this patch too big, I will create NFC patch to clean up the AsmPrinter::emitLinkage.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

432

hasDotLGloblDirective only use here. in order not to let the patch too big . I will remove hasDotLGloblDirective and DotLGloblDirective in new NFC patch.

[DiggerLin]

address comment

[hubert.reinterpretcast]

llvm/include/llvm/MC/MCStreamer.h

568

Still missing "a" before linkage.

573

There was a typo in my suggestion:
s/is/if/;

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1534–1543

Suggestion:

// Handle the XCOFF case.
// `Name` is the function descriptor symbol (see above). Get the function
// entry point symbol.

llvm/lib/MC/MCAsmInfoXCOFF.cpp

20

Move this to right after IsLittleEndian to maintain matching the base class order.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1606

The string should be "Expecting .lglobl to be supported for AIX."

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-xcoff-visibility.ll

52 ↗

(On Diff #269029)

Good catch; thanks.

[DiggerLin]

address comment

[hubert.reinterpretcast]

LGTM with small comment.

llvm/lib/MC/MCAsmInfoXCOFF.cpp

20

? I said to move HasVisibilityOnlyWithLinkage up, not IsLittleEndian down.

IsLittleEndian = false;
HasVisibilityOnlyWithLinkage = true;
PrivateGlobalPrefix = "L..";
PrivateLabelPrefix = "L..";

llvm/include/llvm/BinaryFormat/XCOFF.h

 enum SymbolType : uint8_t {
   XTY_ER = 0, ///< External reference.
   XTY_SD = 1, ///< Csect definition for initialized storage.
   XTY_LD = 2, ///< Label definition.
               ///< Defines an entry point to an initialized csect.
   XTY_CM = 3  ///< Common csect definition. For uninitialized storage.
 };
 
+/// Values for visibility as they would appear when encoded in the high 4 bits
+/// of the 16-bit unsigned n_type field of symbol table entries. Valid for
+/// 32-bit XCOFF only when the vstamp in the auxiliary header is greater than 1.
+enum VisibilityType : uint16_t {
+  SYM_V_UNSPECIFIED = 0x0000,
+  SYM_V_INTERNAL = 0x1000,
+  SYM_V_HIDDEN = 0x2000,
+  SYM_V_PROTECTED = 0x3000,
+  SYM_V_EXPORTED = 0x4000
+};
+
 // Relocation types, defined in `/usr/include/reloc.h`.
 enum RelocationType : uint8_t {
   R_POS = 0x00, ///< Positive relocation. Provides the address of the referenced
                 ///< symbol.
   R_RL = 0x0c,  ///< Positive indirect load relocation. Modifiable instruction.
   R_RLA = 0x0d, ///< Positive load address relocation. Modifiable instruction.
 
   R_NEG = 0x01, ///< Negative relocation. Provides the negative of the address

llvm/include/llvm/CodeGen/AsmPrinter.h

public:
 
   /// This emits visibility information about symbol, if this is supported by
   /// the target.
   void emitVisibility(MCSymbol *Sym, unsigned Visibility,
                       bool IsDefinition = true) const;
 
   /// This emits linkage information about \p GVSym based on \p GV, if this is
   /// supported by the target.
-  void emitLinkage(const GlobalValue *GV, MCSymbol *GVSym) const;
+  virtual void emitLinkage(const GlobalValue *GV, MCSymbol *GVSym) const;
 
   /// Return the alignment for the specified \p GV.
   static Align getGVAlignment(const GlobalValue *GV, const DataLayout &DL,
                               Align InAlign = Align(1));
 
 private:
   /// Private state for PrintSpecial()
   // Assign a unique ID to this machine instruction.

llvm/include/llvm/MC/MCAsmInfo.h

protected:
   /// doesn't handle associative comdats in the way that we would like to use
   /// them.
   bool HasCOFFAssociativeComdats = false;
 
   /// True if this is a non-GNU COFF target. For GNU targets, we don't generate
   /// constants into comdat sections.
   bool HasCOFFComdatConstants = false;
 
+  /// True if this is an XCOFF target that supports visibility attributes as
+  /// part of .global, .weak, .extern, and .comm. Default is false.
+  bool HasVisibilityOnlyWithLinkage = false;
+
   /// This is the maximum possible length of an instruction, which is needed to
   /// compute the size of an inline asm.  Defaults to 4.
   unsigned MaxInstLength = 4;
 
   /// Every possible instruction length is a multiple of this value.  Factored
   /// out in .debug_frame and .debug_line.  Defaults to 1.
   unsigned MinInstAlignment = 1;
 
public:
   }
 
   // Accessors.
 
   bool hasMachoZeroFillDirective() const { return HasMachoZeroFillDirective; }
   bool hasMachoTBSSDirective() const { return HasMachoTBSSDirective; }
   bool hasCOFFAssociativeComdats() const { return HasCOFFAssociativeComdats; }
   bool hasCOFFComdatConstants() const { return HasCOFFComdatConstants; }
+  bool hasVisibilityOnlyWithLinkage() const {
+    return HasVisibilityOnlyWithLinkage;
+  }
 
   /// Returns the maximum possible encoded instruction size in bytes. If \p STI
   /// is null, this should be the maximum size for any subtarget.
   virtual unsigned getMaxInstLength(const MCSubtargetInfo *STI = nullptr) const {
     return MaxInstLength;
   }
 
   unsigned getMinInstAlignment() const { return MinInstAlignment; }

llvm/include/llvm/MC/MCStreamer.h

public:
   /// \param Size - The size of the block of storage.
   /// \param CsectSym - Csect name for the block of storage.
   /// \param ByteAlignment - The alignment of the symbol in bytes. Must be a
   /// power of 2.
   virtual void emitXCOFFLocalCommonSymbol(MCSymbol *LabelSym, uint64_t Size,
                                           MCSymbol *CsectSym,
                                           unsigned ByteAlignment);
 
+  /// Emit a symbol's linkage and visibilty with a linkage directive for XCOFF.
+  ///
+  /// \param Symbol - The symbol to emit.
+  /// \param Linkage - The linkage of the symbol to emit.
+  /// \param Visibility - The visibility of the symbol to emit or MCSA_Invalid
+  /// if the symbol does not have an explicit visibility.
+  virtual void emitXCOFFSymbolLinkageWithVisibility(MCSymbol *Symbol,
+                                                    MCSymbolAttr Linkage,
+                                                    MCSymbolAttr Visibility);
   /// Emit an ELF .size directive.
   ///
   /// This corresponds to an assembler statement such as:
   ///  .size symbol, expression
   virtual void emitELFSize(MCSymbol *Symbol, const MCExpr *Value);
 
   /// Emit an ELF .symver directive.
   ///
   /// This corresponds to an assembler statement such as:

llvm/include/llvm/MC/MCSymbolXCOFF.h

public:
   }
 
   bool hasRepresentedCsectSet() const { return RepresentedCsect != nullptr; }
 
   MCSectionXCOFF *getRepresentedCsect() const;
 
   void setRepresentedCsect(MCSectionXCOFF *C);
 
+  void setVisibilityType(XCOFF::VisibilityType SVT) { VisibilityType = SVT; };
+
+  XCOFF::VisibilityType getVisibilityType() const { return VisibilityType; }
+
 private:
   Optional<XCOFF::StorageClass> StorageClass;
   MCSectionXCOFF *RepresentedCsect = nullptr;
+  XCOFF::VisibilityType VisibilityType = XCOFF::SYM_V_UNSPECIFIED;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_MC_MCSYMBOLXCOFF_H

llvm/include/llvm/MC/MCXCOFFStreamer.h

public:
 
   bool emitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr Attribute) override;
   void emitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
                         unsigned ByteAlignment) override;
   void emitZerofill(MCSection *Section, MCSymbol *Symbol = nullptr,
                     uint64_t Size = 0, unsigned ByteAlignment = 0,
                     SMLoc Loc = SMLoc()) override;
   void emitInstToData(const MCInst &Inst, const MCSubtargetInfo &) override;
   void emitXCOFFLocalCommonSymbol(MCSymbol *LabelSym, uint64_t Size,
                                   MCSymbol *CsectSym,
                                   unsigned ByteAlign) override;
+  void emitXCOFFSymbolLinkageWithVisibility(MCSymbol *Symbol,
+                                            MCSymbolAttr Linkage,
+                                            MCSymbolAttr Visibility) override;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_MC_MCXCOFFSTREAMER_H

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

if (MAI->hasWeakDefDirective()) {
       OutStreamer->emitSymbolAttribute(GVSym, MCSA_Global);
       //NOTE: linkonce is handled by the section the symbol was assigned to.
     } else {
       // .weak _foo
       OutStreamer->emitSymbolAttribute(GVSym, MCSA_Weak);
     }
     return;
   case GlobalValue::ExternalLinkage:
     if (MAI->hasDotExternDirective() && GV->isDeclaration())
       OutStreamer->emitSymbolAttribute(GVSym, MCSA_Extern);
     else
       OutStreamer->emitSymbolAttribute(GVSym, MCSA_Global);
     return;
   case GlobalValue::PrivateLinkage:
     return;
   case GlobalValue::InternalLinkage:
     if (MAI->hasDotLGloblDirective())
       OutStreamer->emitSymbolAttribute(GVSym, MCSA_LGlobal);
     return;
   case GlobalValue::ExternalWeakLinkage:
     if (TM.getTargetTriple().isOSBinFormatXCOFF()) {
       OutStreamer->emitSymbolAttribute(GVSym, MCSA_Weak);
       return;
     }
     LLVM_FALLTHROUGH;
   case GlobalValue::AvailableExternallyLinkage:
     if (MAI->hasDotExternDirective()) {
       OutStreamer->emitSymbolAttribute(GVSym, MCSA_Extern);
       return;
     }
     LLVM_FALLTHROUGH;
   case GlobalValue::AppendingLinkage:
     llvm_unreachable("Should never emit this");
   }
   llvm_unreachable("Unknown linkage type!");
void AsmPrinter::emitFunctionHeader() {
 
   // Print out constants referenced by the function
   emitConstantPool();
 
   // Print the 'header' of function.
   MF->setSection(getObjFileLowering().SectionForGlobal(&F, TM));
   OutStreamer->SwitchSection(MF->getSection());
 
+  if (!MAI->hasVisibilityOnlyWithLinkage())
     emitVisibility(CurrentFnSym, F.getVisibility());
 
   if (MAI->needsFunctionDescriptors() &&
       F.getLinkage() != GlobalValue::InternalLinkage)
     emitLinkage(&F, CurrentFnDescSym);
 
   emitLinkage(&F, CurrentFnSym);
   if (MAI->hasFunctionAlignment())
     emitAlignment(MF->getAlignment(), &F);
bool AsmPrinter::doFinalization(Module &M) {
   const TargetLoweringObjectFile &TLOF = getObjFileLowering();
 
   // Emit linkage(XCOFF) and visibility info for declarations
   for (const Function &F : M) {
     if (!F.isDeclarationForLinker())
       continue;
 
     MCSymbol *Name = getSymbol(&F);
     // Function getSymbol gives us the function descriptor symbol for XCOFF.
-    if (TM.getTargetTriple().isOSBinFormatXCOFF() && !F.isIntrinsic()) {
 
-      // Get the function entry point symbol.
-      MCSymbol *FnEntryPointSym = TLOF.getFunctionEntryPointSymbol(&F, TM);
-      if (cast<MCSymbolXCOFF>(FnEntryPointSym)->hasRepresentedCsectSet())
-        // Emit linkage for the function entry point.
-        emitLinkage(&F, FnEntryPointSym);
+    if (!TM.getTargetTriple().isOSBinFormatXCOFF()) {
+      GlobalValue::VisibilityTypes V = F.getVisibility();
+      if (V == GlobalValue::DefaultVisibility)
+        continue;
+
+      emitVisibility(Name, V, false);
+      continue;
+    }
+
+    if (F.isIntrinsic())
+      continue;
+
+    // Handle the XCOFF case.
+    // Variable `Name` is the function descriptor symbol (see above). Get the
+    // function entry point symbol.
 
-      // Emit linkage for the function descriptor.
-      emitLinkage(&F, Name);
-    }
 
-    GlobalValue::VisibilityTypes V = F.getVisibility();
-    if (V == GlobalValue::DefaultVisibility)
-      continue;
-
-    emitVisibility(Name, V, false);
-  }
-
   // Emit the remarks section contents.
   // FIXME: Figure out when is the safest time to emit this section. It should
   // not come after debug info.
   if (remarks::RemarkStreamer *RS = M.getContext().getMainRemarkStreamer())
     emitRemarksSection(*RS);
 
   TLOF.emitModuleMetadata(*OutStreamer, M);
 

llvm/lib/MC/MCAsmInfoXCOFF.cpp

 //===- MC/MCAsmInfoXCOFF.cpp - XCOFF asm properties ------------ *- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/MC/MCAsmInfoXCOFF.h"
 
 using namespace llvm;
 
 void MCAsmInfoXCOFF::anchor() {}
 
 MCAsmInfoXCOFF::MCAsmInfoXCOFF() {
   IsLittleEndian = false;
+  HasVisibilityOnlyWithLinkage = true;
   PrivateGlobalPrefix = "L..";
   PrivateLabelPrefix = "L..";
   SupportsQuotedNames = false;
   UseDotAlignForAlignment = true;
   ZeroDirective = "\t.space\t";
   ZeroDirectiveSupportsNonZeroValue = false;
   AsciiDirective = nullptr; // not supported
   AscizDirective = nullptr; // not supported
 
   // Use .vbyte for data definition to avoid directives that apply an implicit
   // alignment.

llvm/lib/MC/MCAsmStreamer.cpp

 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstPrinter.h"
 #include "llvm/MC/MCObjectFileInfo.h"
 #include "llvm/MC/MCObjectWriter.h"
 #include "llvm/MC/MCRegister.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCSectionMachO.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/TargetRegistry.h"
 #include <cctype>
 
public:
   void EmitCOFFSafeSEH(MCSymbol const *Symbol) override;
   void EmitCOFFSymbolIndex(MCSymbol const *Symbol) override;
   void EmitCOFFSectionIndex(MCSymbol const *Symbol) override;
   void EmitCOFFSecRel32(MCSymbol const *Symbol, uint64_t Offset) override;
   void EmitCOFFImgRel32(MCSymbol const *Symbol, int64_t Offset) override;
   void emitXCOFFLocalCommonSymbol(MCSymbol *LabelSym, uint64_t Size,
                                   MCSymbol *CsectSym,
                                   unsigned ByteAlign) override;
+  void emitXCOFFSymbolLinkageWithVisibility(MCSymbol *Symbol,
+                                            MCSymbolAttr Linakge,
+                                            MCSymbolAttr Visibility) override;
+
   void emitELFSize(MCSymbol *Symbol, const MCExpr *Value) override;
   void emitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
                         unsigned ByteAlignment) override;
 
   /// Emit a local common (.lcomm) symbol.
   ///
   /// @param Symbol - The common symbol to emit.
   /// @param Size - The size of the common symbol.
void MCAsmStreamer::emitXCOFFLocalCommonSymbol(MCSymbol *LabelSym,
   LabelSym->print(OS, MAI);
   OS << ',' << Size << ',';
   CsectSym->print(OS, MAI);
   OS << ',' << Log2_32(ByteAlignment);
 
   EmitEOL();
 }
 
+void MCAsmStreamer::emitXCOFFSymbolLinkageWithVisibility(
+    MCSymbol *Symbol, MCSymbolAttr Linkage, MCSymbolAttr Visibility) {
+
+  switch (Linkage) {
+  case MCSA_Global:
+    OS << MAI->getGlobalDirective();
+    break;
+  case MCSA_Weak:
+    OS << MAI->getWeakDirective();
+    break;
+  case MCSA_Extern:
+    OS << "\t.extern\t";
+    break;
+  default:
+    report_fatal_error("unhandled linkage type");
+  }
+
+  Symbol->print(OS, MAI);
+
+  switch (Visibility) {
+  case MCSA_Invalid:
+    // Nothing to do.
+    break;
+  case MCSA_Hidden:
+    OS << ",hidden";
+    break;
+  case MCSA_Protected:
+    OS << ",protected";
+    break;
+  default:
+    report_fatal_error("unexpected value for Visibility type");
+  }
+  EmitEOL();
+}
+
 void MCAsmStreamer::emitELFSize(MCSymbol *Symbol, const MCExpr *Value) {
   assert(MAI->hasDotTypeDotSizeDirective());
   OS << "\t.size\t";
   Symbol->print(OS, MAI);
   OS << ", ";
   Value->print(OS, MAI);
   EmitEOL();
 }
 
 void MCAsmStreamer::emitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
                                      unsigned ByteAlignment) {
   OS << "\t.comm\t";
   Symbol->print(OS, MAI);
   OS << ',' << Size;
 
   if (ByteAlignment != 0) {
     if (MAI->getCOMMDirectiveAlignmentIsInBytes())

llvm/lib/MC/MCStreamer.cpp

 void MCStreamer::EmitCOFFSymbolType(int Type) {
   llvm_unreachable("this directive only supported on COFF targets");
 }
 void MCStreamer::emitXCOFFLocalCommonSymbol(MCSymbol *LabelSym, uint64_t Size,
                                             MCSymbol *CsectSym,
                                             unsigned ByteAlign) {
   llvm_unreachable("this directive only supported on XCOFF targets");
 }
+
+void MCStreamer::emitXCOFFSymbolLinkageWithVisibility(MCSymbol *Symbol,
+                                                      MCSymbolAttr Linkage,
+                                                      MCSymbolAttr Visibility) {
+  llvm_unreachable("emitXCOFFSymbolLinkageWithVisibility is only supported on "
+                   "XCOFF targets");
+}
+
 void MCStreamer::emitELFSize(MCSymbol *Symbol, const MCExpr *Value) {}
 void MCStreamer::emitELFSymverDirective(StringRef AliasName,
                                         const MCSymbol *Aliasee) {}
 void MCStreamer::emitLocalCommonSymbol(MCSymbol *Symbol, uint64_t Size,
                                        unsigned ByteAlignment) {}
 void MCStreamer::emitTBSSSymbol(MCSection *Section, MCSymbol *Symbol,
                                 uint64_t Size, unsigned ByteAlignment) {}
 void MCStreamer::changeSection(MCSection *, const MCExpr *) {}

llvm/lib/MC/MCXCOFFStreamer.cpp

bool MCXCOFFStreamer::emitSymbolAttribute(MCSymbol *Sym,
   case MCSA_LGlobal:
     Symbol->setStorageClass(XCOFF::C_HIDEXT);
     Symbol->setExternal(true);
     break;
   case llvm::MCSA_Weak:
     Symbol->setStorageClass(XCOFF::C_WEAKEXT);
     Symbol->setExternal(true);
     break;
+  case llvm::MCSA_Hidden:
+    Symbol->setVisibilityType(XCOFF::SYM_V_HIDDEN);
+    break;
+  case llvm::MCSA_Protected:
+    Symbol->setVisibilityType(XCOFF::SYM_V_PROTECTED);
+    break;
   default:
     report_fatal_error("Not implemented yet.");
   }
   return true;
 }
 
+void MCXCOFFStreamer::emitXCOFFSymbolLinkageWithVisibility(
+    MCSymbol *Symbol, MCSymbolAttr Linkage, MCSymbolAttr Visibility) {
+
+  emitSymbolAttribute(Symbol, Linkage);
+
+  // When the caller passes `MCSA_Invalid` for the visibility, do not emit one.
+  if (Visibility == MCSA_Invalid)
+    return;
+
+  emitSymbolAttribute(Symbol, Visibility);
+}
+
 void MCXCOFFStreamer::emitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
                                        unsigned ByteAlignment) {
   getAssembler().registerSymbol(*Symbol);
   Symbol->setExternal(cast<MCSymbolXCOFF>(Symbol)->getStorageClass() !=
                       XCOFF::C_HIDEXT);
   Symbol->setCommon(Size, ByteAlignment);
 
   // Default csect align is 4, but common symbols have explicit alignment values

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

public:
 
   void SetupMachineFunction(MachineFunction &MF) override;
 
   void emitGlobalVariable(const GlobalVariable *GV) override;
 
   void emitFunctionDescriptor() override;
 
   void emitEndOfAsmFile(Module &) override;
+
+  void emitLinkage(const GlobalValue *GV, MCSymbol *GVSym) const override;
 };
 
 } // end anonymous namespace
 
 void PPCAsmPrinter::PrintSymbolOperand(const MachineOperand &MO,
                                        raw_ostream &O) {
   // Computing the address of a global symbol, not calling it.
   const GlobalValue *GV = MO.getGlobal();
void PPCLinuxAsmPrinter::emitFunctionBodyEnd() {
   // the PPC64 ELF ABI (this is a low-priority item because GDB does not
   // currently make use of these fields).
   if (Subtarget->isPPC64()) {
     OutStreamer->emitIntValue(0, 4/*size*/);
     OutStreamer->emitIntValue(0, 8/*size*/);
   }
 }
 
+void PPCAIXAsmPrinter::emitLinkage(const GlobalValue *GV,
+                                   MCSymbol *GVSym) const {
+
+  assert(MAI->hasVisibilityOnlyWithLinkage() &&
+         "AIX's linkage directives take a visibility setting.");
+
+  MCSymbolAttr LinkageAttr = MCSA_Invalid;
+  switch (GV->getLinkage()) {
+  case GlobalValue::ExternalLinkage:
+    LinkageAttr = GV->isDeclaration() ? MCSA_Extern : MCSA_Global;
+    break;
+  case GlobalValue::LinkOnceAnyLinkage:
+  case GlobalValue::LinkOnceODRLinkage:
+  case GlobalValue::WeakAnyLinkage:
+  case GlobalValue::WeakODRLinkage:
+  case GlobalValue::ExternalWeakLinkage:
+    LinkageAttr = MCSA_Weak;
+    break;
+  case GlobalValue::AvailableExternallyLinkage:
+    LinkageAttr = MCSA_Extern;
+    break;
+  case GlobalValue::PrivateLinkage:
+    return;
+  case GlobalValue::InternalLinkage:
+    assert(MAI->hasDotLGloblDirective() &&
+           "Expecting .lglobl to be supported for AIX.");
+    OutStreamer->emitSymbolAttribute(GVSym, MCSA_LGlobal);
+    return;
+  case GlobalValue::AppendingLinkage:
+    llvm_unreachable("Should never emit this");
+  case GlobalValue::CommonLinkage:
+    llvm_unreachable("CommonLinkage of XCOFF should not come to this path");
+  }
+
+  assert(LinkageAttr != MCSA_Invalid && "LinkageAttr should not MCSA_Invalid.");
+
+  MCSymbolAttr VisibilityAttr = MCSA_Invalid;
+  switch (GV->getVisibility()) {
+
+    // TODO: "exported" and "internal" Visibility needs to go here.
+
+  case GlobalValue::DefaultVisibility:
+    break;
+  case GlobalValue::HiddenVisibility:
+    VisibilityAttr = MAI->getHiddenVisibilityAttr();
+    break;
+  case GlobalValue::ProtectedVisibility:
+    VisibilityAttr = MAI->getProtectedVisibilityAttr();
+    break;
+  }
+
+  OutStreamer->emitXCOFFSymbolLinkageWithVisibility(GVSym, LinkageAttr,
+                                                    VisibilityAttr);
+}
+
 void PPCAIXAsmPrinter::SetupMachineFunction(MachineFunction &MF) {
   // Setup CurrentFnDescSym and its containing csect.
   MCSectionXCOFF *FnDescSec =
       cast<MCSectionXCOFF>(getObjFileLowering().getSectionForFunctionDescriptor(
           &MF.getFunction(), TM));
   FnDescSec->setAlignment(Align(Subtarget->isPPC64() ? 8 : 4));
 
   CurrentFnDescSym = FnDescSec->getQualNameSymbol();