Page MenuHomePhabricator

[XCOFF] write the aux header when the visibility is specified in XCOFF32.
ClosedPublic

Authored by Esme on Jun 19 2022, 7:14 PM.

Details

Summary

The n_type field in the symbol table entry has two interpretations in XCOFF32, and a single interpretation in XCOFF64.
The new interpretation is used in XCOFF32 if the value of the o_vstamp field in the auxiliary header is 2.
In XCOFF64 and the new XCOFF32 interpretation, the n_type field is used for the symbol type and visibility.

The patch writes the aux header with an o_vstamp field value of 2 when the visibility is specified in XCOFF32 to make the new XCOFF32 interpretation used.

Diff Detail

Event Timeline

Esme created this revision.Jun 19 2022, 7:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2022, 7:14 PM
Esme requested review of this revision.Jun 19 2022, 7:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2022, 7:14 PM
DiggerLin added inline comments.Jun 20 2022, 9:51 AM
llvm/include/llvm/BinaryFormat/XCOFF.h
34

not sure how the size 28 come from ?

llvm/lib/MC/XCOFFObjectWriter.cpp
314–315

according to "The auxiliary header contains system-dependent and implementation-dependent information, in which is used for loading and executing a module. Information in the auxiliary header minimizes how much of the file must be processed by the system loader at execution time." in https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format#XCOFF__fyovh386shar

the auxiliaryHeaderSize should not depend on the HasVisibility

785

in the xcoff.h , there is
enum XCOFFInterpret : uint16_t {

OLD_XCOFF_INTERPRET = 1,
NEW_XCOFF_INTERPRET = 2

};

maybe better to use NEW_XCOFF_INTERPRET than 2 .

Esme added inline comments.Jun 21 2022, 12:40 AM
llvm/include/llvm/BinaryFormat/XCOFF.h
34
Some AIX programs generate auxiliary headers for 32-bit object files that end after the data_start field.  The macro _AOUTHSZ_SHORT defines the length of such a header.

As I replied in the next comment, the value of _AOUTHSZ_SHORT here is 28.
Well, changing AuxFileHeaderSizeObj to AuxFileHeaderSizeShort might be more appropriate?

llvm/lib/MC/XCOFFObjectWriter.cpp
314–315

Thank you for your comments!
Generally, the auxiliary header should not be required by an un-executable object file. However, it may be needed in some specific cases, like the visibility case here.
Here is a related description in /usr/include/aouthdr.h of AIX.

In general, the auxiliary header can be any size, and the length of the auxiliary in any particular object file is specified in the f_opthdr field of the XCOFF header.
Some AIX programs generate auxiliary headers for 32-bit object files that end after the data_start field.  The macro _AOUTHSZ_SHORT defines the length of such a header.  No auxiliary header is required for an object file that is not an executable.
Esme updated this revision to Diff 438627.Jun 21 2022, 3:44 AM
DiggerLin added inline comments.Jun 29 2022, 1:28 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
468

maybe more readable to change to
if (XSym->getVisibilityType() !=XCOFF::SYM_V_UNSPECIFIED)

771

since in function auxiliaryHeaderSize() , 64bit return size zero, can we use auxiliaryHeaderSize() here ?

794

since we can know the address and Size from stuct SectionEntry , do you want to put the value here instead of zero?

Esme updated this revision to Diff 441581.Jun 30 2022, 8:25 PM
Esme marked 3 inline comments as done.

Address comments.

llvm/lib/MC/XCOFFObjectWriter.cpp
794

I may prefer to post another patch for calculating the start addresses and the total sizes for Text/Data/BSS sections since this patch comes from the visibility requirement and these fields are not required.

Esme updated this revision to Diff 442272.Jul 5 2022, 6:12 AM

Addressed comments.
Hi @DiggerLin, please ignore my previous comments.
Since those values of size and address are easy to obtain, I've filled them for the aux header in this revision.

DiggerLin added inline comments.Jul 7 2022, 11:19 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
314–315

add some comment here :
// TODO: not support 64-bit auxiliary header.

784

since we only implement the 32-bit auxiliary header here
it is better to add a new function

writeAuxFileHeader32() {
    W.write<uint16_t>(0); // Magic
   ... 
}

and copy the code into the function.

 void XCOFFObjectWriter::writeAuxFileHeader() {
   if (!auxiliaryHeaderSize())
    return;
   if(!is64Bit()) 
     writeAuxFileHeader32();
  //TODO : 64-bit auxiliary header not implemented.   
}
DiggerLin added inline comments.Jul 7 2022, 1:57 PM
llvm/test/CodeGen/PowerPC/aix-xcoff-visibility.ll
7

change to --check-prefixes=SYM,AUX32 ?

10

change to --check-prefixes=SYM,AUX64 ?

Esme updated this revision to Diff 444476.Jul 13 2022, 6:04 PM
Esme marked 2 inline comments as done.

Addressed.

llvm/lib/MC/XCOFFObjectWriter.cpp
314–315

As the definition in aouthdr.h, 64-bit object files will not have the auxiliary header. So this should not be a TODO?

jhenderson added inline comments.Jul 13 2022, 11:42 PM
llvm/test/CodeGen/PowerPC/aix-aux-header.ll
2

This test is failing in the pre-merge checks. Please make sure to fix it.

Esme updated this revision to Diff 444575.Jul 14 2022, 3:07 AM

@jhenderson Thanks for finding it.

DiggerLin added inline comments.Mon, Jul 18, 1:16 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
314–315

thanks for the clarify. do you want to add some comment for it (64-bit object files will not have the auxiliary header)?

Esme updated this revision to Diff 445698.Mon, Jul 18, 11:01 PM

Add a comment about 64-bit object has no aux header.

DiggerLin accepted this revision.Tue, Jul 19, 6:48 AM

Look good to me. Maybe James Henderson has further comment.

llvm/lib/MC/XCOFFObjectWriter.cpp
314–315

nit: move comment in side the function

// 64-bit object files have no auxiliary header.
 return HasVisibility && !is64Bit() ? XCOFF::AuxFileHeaderSizeShort : 0;
This revision is now accepted and ready to land.Tue, Jul 19, 6:48 AM
jhenderson accepted this revision.Tue, Jul 19, 6:53 AM

I've got nothing else to add. LGTM.

This revision was landed with ongoing or failed builds.Wed, Jul 20, 4:11 AM
This revision was automatically updated to reflect the committed changes.