Page MenuHomePhabricator

[AIX] supporting the visibility attribute for aix assembly
ClosedPublic

Authored by DiggerLin on Mar 9 2020, 12:02 PM.

Details

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)++;
}
  1. the visibility of .comm linkage do not support , we will have a separate patch for it.
  2. we have the unsupported cases ("default" and "internal") , we will implement them in a a separate patch for it.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
DiggerLin added inline comments.May 29 2020, 1:30 PM
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 marked 2 inline comments as done.May 29 2020, 1:31 PM
DiggerLin added inline comments.
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
1615

sorry, please discard above comment.

DiggerLin updated this revision to Diff 267360.May 29 2020, 1:48 PM

address comment

jasonliu added inline comments.Jun 1 2020, 8:31 AM
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 edited the summary of this revision. (Show Details)Jun 2 2020, 12:51 PM
DiggerLin marked 14 inline comments as done.Jun 2 2020, 1:42 PM
DiggerLin added inline comments.
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 marked 4 inline comments as done.Jun 2 2020, 1:43 PM
DiggerLin added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-hidden.ll
56 ↗(On Diff #262418)

added into the patch's summary

DiggerLin updated this revision to Diff 267989.Jun 2 2020, 2:01 PM

address comment

jasonliu added inline comments.Jun 2 2020, 2:30 PM
llvm/lib/MC/MCAsmStreamer.cpp
799

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

jasonliu added inline comments.Jun 3 2020, 10:14 AM
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 marked 4 inline comments as done.Jun 3 2020, 1:56 PM
DiggerLin added inline comments.
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 updated this revision to Diff 268485.Jun 4 2020, 8:20 AM
DiggerLin marked 2 inline comments as done.

address comment

jasonliu accepted this revision.Jun 4 2020, 10:20 AM

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?

This revision is now accepted and ready to land.Jun 4 2020, 10:20 AM
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?

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 marked 27 inline comments as done.Jun 5 2020, 8:01 AM
DiggerLin added inline comments.
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 updated this revision to Diff 268848.Jun 5 2020, 9:35 AM
DiggerLin marked an inline comment as done.

rebase the patch and address comment

jasonliu added inline comments.Jun 5 2020, 10:41 AM
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 marked 2 inline comments as done.Jun 6 2020, 1:55 PM

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 marked an inline comment as done.Jun 6 2020, 1:55 PM
DiggerLin updated this revision to Diff 269029.Jun 6 2020, 2:04 PM

address comment

DiggerLin marked 3 inline comments as done.Jun 6 2020, 2:05 PM
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."

llvm/test/CodeGen/PowerPC/aix-xcoff-visibility.ll
52 ↗(On Diff #269029)

Good catch; thanks.

DiggerLin updated this revision to Diff 269331.Jun 8 2020, 1:06 PM
DiggerLin marked 5 inline comments as done.

address comment

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..";
This revision was automatically updated to reflect the committed changes.
DiggerLin marked an inline comment as done.