This is an archive of the discontinued LLVM Phabricator instance.

[AIX][XCOFF] emit symbol visibility for xcoff object file.
ClosedPublic

Authored by DiggerLin on Jul 21 2020, 1:16 PM.

Diff Detail

Event Timeline

DiggerLin created this revision.Jul 21 2020, 1:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2020, 1:16 PM
Herald added a subscriber: wuzish. · View Herald Transcript
jasonliu added inline comments.Jul 23 2020, 12:18 PM
llvm/lib/MC/MCXCOFFStreamer.cpp
39 ↗(On Diff #279619)

Some thoughts on how to implement this for object generation:
If we want to follow existing practice, the closest thing we could find is StorageClass.
Both symbol and section have storage class, but how we set them are different.
For sections, we set the storage class when we create them. For symbols, we set it using emitSymbolAttribute.
So if we want to follow existing practice, we want to set the visibility with getXCOFFSection.

But if we want to break the existing practice (which could be a bigger change), we could consider using getQualNameSymbol as proxy of the MCSection.
So we only set attribute or property on MCSymbolXCOFF when possible, and in obj gen, we use getQualNameSymbol to get the property that we need for the section.
But the problem with that is the external symbols. We use external function entry point symbols as if they are labels, but in fact, they are csects. So getQualNameSymbol for an external function entry point symbol would not return the symbol that have the correct settings on it. So maybe this is one motivation for us to change .bl .foo to .bl .foo[PR] for external symbols.

As it is right now, if we seeking for minimum work and keep things consistent, we could set the visibility using getXCOFFSection. And if later we think it's important enough to pursue using getQualNameSymbol as proxy of the MCSection, we could do a refactoring at that time?

DiggerLin marked 2 inline comments as done.Jul 27 2020, 10:18 AM
DiggerLin added inline comments.
llvm/lib/MC/MCXCOFFStreamer.cpp
39 ↗(On Diff #279619)

there are

XCOFF::StorageMappingClass MappingClass;
XCOFF::SymbolType Type;
XCOFF::StorageClass StorageClass;
XCOFF::VisibilityType VisibilityType = XCOFF::SYM_V_UNSPECIFIED;

in the MCSectionXCOFF class,
the four attribute only used in the XCOFFObjectWriter, (asm path do not need the StorageClass)

we need get the value of StorageClass, Type,MappingClass before we invoke the getXCOFFSection every time.

actually for the StorageClass, we can set the value of StorageClass when set the
MCXCOFFStreamer::emitSymbolAttribute(). (for common link we can set at MCXCOFFStreamer::emitCommonSymbol).

if we want to refactor StorageClass later, I do not think we need to consistent with StorageClass in this patch. we can do NFC after this patch for StorageClass.

jasonliu added inline comments.Jul 27 2020, 10:49 AM
llvm/lib/MC/MCXCOFFStreamer.cpp
39 ↗(On Diff #279619)

I don't want the setting of MCSectionXCOFF to be different for different property.
So if we do not want to pursue the proxy of MCSymbolXCOFF for now, we should make them consistent.
Or we could do the refactoring first.
This middle state is not what we want.

jasonliu added inline comments.Aug 12 2020, 8:12 AM
llvm/include/llvm/MC/MCSectionXCOFF.h
67

nit: Address Lint comment.

llvm/lib/MC/XCOFFObjectWriter.cpp
568–571

These comments need to be removed or updated.

604–607

Same as above.

address comment

jasonliu added inline comments.Aug 13 2020, 7:42 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
570–571

Space is needed after ','
Suggestion for comments: ... we only set bits 0-3 for symbol's visibility and leave other bits to zero.

605–607

Remove this line.

606

Keep it in sync with the above comments.

jasonliu accepted this revision.Aug 17 2020, 1:43 PM

LGTM.

This revision is now accepted and ready to land.Aug 17 2020, 1:43 PM
This revision was landed with ongoing or failed builds.Aug 21 2020, 8:01 AM
This revision was automatically updated to reflect the committed changes.
llvm/test/CodeGen/PowerPC/aix-xcoff-visibility.ll
76

Generating visibility in XCOFF32 requires the auxiliary header. The test needs to check the auxiliary header. @DiggerLin, @jasonliu, fya.

llvm/lib/MC/XCOFFObjectWriter.cpp
570

This should be expressed as the "four most significant bits" instead of "bits 0-3" to avoid confusion in terms of bit numbering convention.

606

Same comment.

DiggerLin marked an inline comment as done.Aug 31 2021, 7:34 AM
DiggerLin added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-visibility.ll
76
  1. do we want to generate old 32bits xcoff object file format? if we only want to generate the new 32bits xcoff object file and 64 bit xcoff object file. we can use new 32bits and 64 bits xcoff's interpret to generate the n_type here and and set the o_vstamp field to 2 when we generate the auxiliary header later
  1. for decode symbol table entry , the n_type , if we want to decode the old 32 bits xcoff object file. we need to parse the o_vstamp of auxiliary header first and interpret the n_type based on the value of o_vstamp .

@hubert.reinterpretcast