This is an archive of the discontinued LLVM Phabricator instance.

[AIX][XCOFF] Support 64-bit relocation writing and related tests
AcceptedPublic

Authored by MaryamBen on Jun 21 2021, 7:23 AM.

Details

Summary

Add support to 64-bit relocation writing and related tests.
This patch is part of D103696 partition

Diff Detail

Event Timeline

MaryamBen requested review of this revision.Jun 21 2021, 7:23 AM
MaryamBen created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2021, 7:23 AM
MaryamBen edited the summary of this revision. (Show Details)Jun 22 2021, 12:30 AM

This patch from the description is about 64-bit relocation support, so I'm not sure why there's so much changing in the testing. You shouldn't need so many tests for ensuring you have 64-bit relocation support, which implies that the existing tests aren't well designed - they are testing things in a too catch-all manner, and should probably be rewritten to test a small subset of functionality each. Furthermore, why does every test need to cover 64-bit behaviour?

llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp
61

I'm not a PPC/XCOFF/AIX person, so I might be talking nonsense, but it seems like this part of the patch (i.e. the file) isn't really about 64-bit relocation support in XCOFF files?

llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
335

Nit: Line things up for readability.

That being said, are you able to share these checks with the above set, so that you don't have to have the details listed out twice? If necessary, you could use --check-prefixes to specify multiple prefixes so that you can have common parts, and then separate parts for where the two sets differ.

The same sort of comments apply in the other tests too.

347

If I'm not mistaken, you should capture the .file symbol index, rather than this one and work from there.

MaryamBen marked 2 inline comments as done.Jun 23 2021, 7:23 AM
MaryamBen added inline comments.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp
61

This part allow us to get the value of SignAndSize field.
fixup_ppc_half16ds is used in 64-bit and fixup_ppc_half16 in 32-bit.

struct XCOFFRelocation {
  uint32_t SymbolTableIndex;
  uint64_t FixupOffsetInCsect;
  uint8_t **SignAndSize**;
  uint8_t Type;
};
llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
347

The source filename part isn't implemented yet even for 32-bit.

MaryamBen updated this revision to Diff 353965.Jun 23 2021, 7:34 AM
MaryamBen marked 2 inline comments as done.
MaryamBen marked an inline comment as done.Jun 23 2021, 7:38 AM
MaryamBen added inline comments.
llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
335

Thank you for your advice. I have updated all the tests.

MaryamBen marked an inline comment as done.Jun 23 2021, 7:51 AM

This patch from the description is about 64-bit relocation support, so I'm not sure why there's so much changing in the testing. You shouldn't need so many tests for ensuring you have 64-bit relocation support, which implies that the existing tests aren't well designed - they are testing things in a too catch-all manner, and should probably be rewritten to test a small subset of functionality each. Furthermore, why does every test need to cover 64-bit behaviour?

Since I had the fatal error report in 64-bit, I couldn't check the file/section header and symbol table tests. Once I removed this part of the code, I had to implement tests even for the other parts.

report_fatal_error("64-bit XCOFF object files are not supported yet.");

So maybe I could just change the description or do you have another suggestion ?

This patch from the description is about 64-bit relocation support, so I'm not sure why there's so much changing in the testing. You shouldn't need so many tests for ensuring you have 64-bit relocation support, which implies that the existing tests aren't well designed - they are testing things in a too catch-all manner, and should probably be rewritten to test a small subset of functionality each. Furthermore, why does every test need to cover 64-bit behaviour?

Since I had the fatal error report in 64-bit, I couldn't check the file/section header and symbol table tests. Once I removed this part of the code, I had to implement tests even for the other parts.

report_fatal_error("64-bit XCOFF object files are not supported yet.");

So maybe I could just change the description or do you have another suggestion ?

Could most of the test be rewritten to not require relocations? It seems like relocations shouldn't be necessary in many situations (assuming they work similarly to how they do in ELF). That would allow those tests to be updated as you make the other patches in the test series, which in turn provides accompanying testing for them.

llvm/lib/MC/XCOFFObjectWriter.cpp
884

Nit: clang-format.

llvm/test/CodeGen/PowerPC/aix-xcoff-toc.ll
14

You may wish to add --match-full-lines to here and the equvialent 32-bit FileCheck command. There's the potential for a false pass in lines like the following:

; SYM64:         SymbolAlignmentLog2: 3

That line could match any of the following output:

SymbolAlignmentLog2: 3
SymbolAlignmentLog2: 32
SymbolAlignmentLog2: 32131241251

etc.

97

Here and throughout, this should be SYM32-NEXT (or SYM64-NEXT), right?

MaryamBen updated this revision to Diff 354482.Jun 25 2021, 6:26 AM
MaryamBen marked an inline comment as done.

This patch from the description is about 64-bit relocation support, so I'm not sure why there's so much changing in the testing. You shouldn't need so many tests for ensuring you have 64-bit relocation support, which implies that the existing tests aren't well designed - they are testing things in a too catch-all manner, and should probably be rewritten to test a small subset of functionality each. Furthermore, why does every test need to cover 64-bit behaviour?

Since I had the fatal error report in 64-bit, I couldn't check the file/section header and symbol table tests. Once I removed this part of the code, I had to implement tests even for the other parts.

report_fatal_error("64-bit XCOFF object files are not supported yet.");

So maybe I could just change the description or do you have another suggestion ?

Could most of the test be rewritten to not require relocations? It seems like relocations shouldn't be necessary in many situations (assuming they work similarly to how they do in ELF). That would allow those tests to be updated as you make the other patches in the test series, which in turn provides accompanying testing for them.

From what I know there is no way to ensure testing without relocations, even for a basic code.
An XCOFF/AIX person might confirm !

MaryamBen updated this revision to Diff 354500.Jun 25 2021, 7:50 AM

I'm not going to be able to contribute much more to this or the related reviews - an XCOFF developer will need to give the sign off on them.

llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
89

Here and everywhere else you've used CHECKSYM32 or CHECKSYM64 - should they be CHECKSYM32-NEXT and CHECKSYM64-NEXT?

Same goes in other similar cases - make sure you to use the -NEXT suffix wherever possible.

MaryamBen updated this revision to Diff 355542.Jun 30 2021, 7:31 AM
MaryamBen marked an inline comment as done.Jun 30 2021, 7:33 AM
MaryamBen added inline comments.
llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
89

Thank you, I applied this to all the patchs.

MaryamBen updated this revision to Diff 361606.Jul 26 2021, 3:10 AM
MaryamBen marked an inline comment as done.
MaryamBen marked an inline comment as done.Jul 26 2021, 4:35 AM
MaryamBen added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-toc.ll
14

In some cases the abstraction is used to combine the 32/64 bit.

If you think it's better, I will add it whenever it's possible.

jhenderson added inline comments.Jul 27 2021, 12:23 AM
llvm/test/CodeGen/PowerPC/aix-xcoff-toc.ll
14

In some cases the abstraction is used to combine the 32/64 bit.

I'm not sure I follow how that impacts the comment I made, could you clarify? There's nothing wrong with the use of the prefix.

I'm suggesting your FileCheck command becomes: FileCheck --check-prefixes SYM,SYM64 %s --match-full-lines and FileCheck --check-prefixes SYM,SYM32 %s similarly.

MaryamBen marked 2 inline comments as done.Jul 27 2021, 7:59 AM
MaryamBen added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-toc.ll
14

For symbols there is no problem.

I thought you wanted me to add --match-full-line to every FileCheck which is not possible in the following cases:

1-When we check just the beginning:
; CHECK: .comm globa[RW],4,2
the real value is .comm globa[RW],4,2 # @globa

2- Avoid writing address in 64 bit :
; DIS: 00000000 <.text>:
instead of 0000000000000000 <.text>:

MaryamBen updated this revision to Diff 362031.Jul 27 2021, 8:00 AM
MaryamBen marked an inline comment as done.
jhenderson accepted this revision.Jul 27 2021, 11:50 PM

Looks reasonable, but I think you need to get an XCOFF person to sign off too before pushing this.

This revision is now accepted and ready to land.Jul 27 2021, 11:50 PM
Helflym accepted this revision.Aug 6 2021, 1:30 AM
Helflym added a subscriber: Helflym.

LGTM for the XCOFF part