Add support to 64-bit relocation writing and related tests.
This patch is part of D103696 partition
Details
- Reviewers
jhenderson nemanjai sfertile Esme jasonliu daltenty hubert.reinterpretcast Helflym - Group Reviewers
Restricted Project
Diff Detail
Unit Tests
Event Timeline
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 | ||
360 | 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. | |
372 | If I'm not mistaken, you should capture the .file symbol index, rather than this one and work from there. |
llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp | ||
---|---|---|
61 | This part allow us to get the value of SignAndSize field. struct XCOFFRelocation { uint32_t SymbolTableIndex; uint64_t FixupOffsetInCsect; uint8_t **SignAndSize**; uint8_t Type; }; | |
llvm/test/CodeGen/PowerPC/aix-extern-weak.ll | ||
372 | The source filename part isn't implemented yet even for 32-bit. |
llvm/test/CodeGen/PowerPC/aix-extern-weak.ll | ||
---|---|---|
360 | Thank you for your advice. I have updated all the tests. |
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? |
From what I know there is no way to ensure testing without relocations, even for a basic code.
An XCOFF/AIX person might confirm !
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–91 | 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. |
llvm/test/CodeGen/PowerPC/aix-extern-weak.ll | ||
---|---|---|
89–91 | Thank you, I applied this to all the patchs. |
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. |
llvm/test/CodeGen/PowerPC/aix-xcoff-toc.ll | ||
---|---|---|
14 |
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. |
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: 2- Avoid writing address in 64 bit : |
Looks reasonable, but I think you need to get an XCOFF person to sign off too before pushing this.
clang-format: please reformat the code