[lld][AArch64][ELF][PAC] Support .relr.auth.dyn section
#87635
Conversation
Support `R_AARCH64_AUTH_RELATIVE` relocation compression as described in https://github.com/ARM-software/abi-aa/blob/main/pauthabielf64/pauthabielf64.rst#relocation-compression
kovdan01
requested review from
asl,
ahmedbougacha,
MaskRay,
pcc,
TNorthover and
smithp35
|
@MaskRay Would be glad to see your feedback on this. It would be nice to have RELR support merged since it's present in the spec and CREL is not widely adopted and accepted yet, and having some way to reduce relative relocs size is a desirable feature. |
|
@MaskRay A kind reminder regarding the PR |
lld/ELF/Arch/AArch64.cpp
Outdated
| // .relr.auth.dyn to .rela.dyn, and the addend write is not needed. | ||
| // | ||
| // If val fits in 32 bits, we have two potential scenarios: | ||
| // * True RELR: Write the 32-bit `val` |
There was a problem hiding this comment.
Added, see e68efcf
lld/ELF/SyntheticSections.h
Outdated
| @@ -544,7 +544,9 @@ class RelocationBaseSection : public SyntheticSection { | |||
| static bool classof(const SectionBase *d) { | |||
| return SyntheticSection::classof(d) && | |||
| (d->type == llvm::ELF::SHT_RELA || d->type == llvm::ELF::SHT_REL || | |||
| d->type == llvm::ELF::SHT_RELR); | |||
| d->type == llvm::ELF::SHT_RELR || | |||
| (config->emachine == llvm::ELF::EM_AARCH64 && | |||
There was a problem hiding this comment.
check d->type before config->emachine
There was a problem hiding this comment.
Fixed, thanks, see e68efcf
lld/ELF/Writer.cpp
Outdated
| reloc.offset, | ||
| DynamicReloc::AddendOnlyWithTargetVA, | ||
| *reloc.sym, reloc.addend, R_ABS}); | ||
| // See also AArch64::relocate |
There was a problem hiding this comment.
This comment can be moved to the previous one or just dropped.
There was a problem hiding this comment.
Moved to the previous one, see e68efcf
lld/test/ELF/aarch64-reloc-pauth.s
Outdated
|
|
||
| # EMPTY-RELA: Section Headers: | ||
| # EMPTY-RELA-NEXT: Name Type Address Off Size ES Flg Lk Inf Al | ||
| # EMPTY-RELA: .rela.dyn RELA {{0*}}[[ADDR1:.*]] {{0*}}[[ADDR1]] 000000 18 A 0 0 8 |
There was a problem hiding this comment.
[[#%x,ADDR1]] and {{0*}}[[#ADDR1]] below.
Better to avoid .*
There was a problem hiding this comment.
I'm not sure if we can use numeric variables here - see the following limitation https://www.llvm.org/docs/CommandGuide/FileCheck.html#filecheck-numeric-substitution-blocks
Important note: In its current implementation, an expression cannot use a numeric variable defined earlier in the same CHECK directive.
We need to use ADDR1 and ADDR2 on the same check lines where they are defined. FileCheck fails with error: numeric variable 'ADDR1' defined earlier in the same CHECK directive.
Do I miss something or we can leave this "as is" due to this FileCheck limitation?
There was a problem hiding this comment.
In this case it is fine. .+ is still probably better than .* (which can match spaces)
There was a problem hiding this comment.
Changed .* to .+, thanks for suggestion, see 8a83e05
|
We started seeing an lld relocation issue, and I bisected to this commit. This is just heads up, and I'll try to come up with a small reproducer. I'm just posting early in case you have any idea. |
@gulfemsavrun With limited information, I suspect the root cause is something else. The lld error is to disallow a relocation section with an invalid target . You can run |
I did a few more testing yesterday and I can comfirm, with this change, lld generated an object file with an empty REL section with Without this change: With this change: You can see index 16, |
|
@zeroomega Thanks for a detailed report!
Having rel(a) or relr section empty in some rare cases is expected behavior with this patch applied, we have tests for that (unfortunately, it's pretty hard to avoid that at current lld state). But, if such files cause issues when passed later as input for lld, we need to investigate that. If it does not contradict with specs, lld should be able to properly process input files with empty relocation sections, and it looks like that it isn't able to do that now. |
I think the main issue is not an empty REL(A) section. This patch causes lld to generate a REL section with |
|
I have generated a small reproducer package: Steps to reproduce: You should see the Since this patch introduced an issue that lld fail to link the file it generated. Could we revert this change? Other people who uses relocatable linking on aarch64 will likely hit the same issue once they upgrade their lld. |
Well, it is great that this issue was found. So, it should be treated as normal bug / missed functionality. Normally per developer policy we do not revert unless it breaks the bot or is known regression wrt some test existing in the mainline. |
|
This is not at all the policy normally observed in LLVM. Breakages that are not found by existing tests cannot be allowed to persist just because the test didn't exist before the breakage was landed. It is quite normal for downstream reports of an actual regression in a real world use to be reason to revert. It's not a sign that your change wasn't broken. It's a sign that there was already insufficient testing, and your change did not add sufficient testing, to catch the real regression. It is invalid output ever to produce an There is a lack of existing testing coverage for |
I don't think the revert policy you mentioned is correct.
In this particular case. lld now generates invalid output (it rejects its output as its input) on AArch64 (a supported target) with relocatable linking (a supported feature). We happened to discovered it in field. This should have covered by lld tests but it such tests did not exist in the current LLVM. I think it is a valid request to revert this change, fix it, combined with additional testing and re-land it. It is a correctness issue which is now observed and validated. |
…94843) Reverts #87635 On some corner cases, lld generated an object file with an empty REL section with `sh_info` set to 0. This file triggers an lld error when used as its input. See #87635 (comment) for details.
|
@zeroomega @gulfemsavrun Reverted changes, see #94843. @zeroomega Thanks for providing a reproducer |
…lvm#94843) Reverts llvm#87635 On some corner cases, lld generated an object file with an empty REL section with `sh_info` set to 0. This file triggers an lld error when used as its input. See llvm#87635 (comment) for details. Signed-off-by: Hafidz Muzakky <ais.muzakky@gmail.com>
This re-applies llvm#87635 with fix of issue described in llvm#87635 (comment). We need to discard `rel(a).dyn` section when output is a relocatable object file. The section is always empty in such a case (as well as both auth and regular relr sections), and emitting that resulted in its `sh_info` equal to 0. Section with zero index is always a NULL section according to ELF spec, and lld is unable to run `ObjFile<ELFT>::getRelocTarget` against relocation section with `sh_info` equal to zero (the ELF spec does not seem to define behavior for such a corner case). This is only an issue with relocatable object file output - having `sh_info` equal to zero for shared objects is OK. Original commit description below. Support `R_AARCH64_AUTH_RELATIVE` relocation compression as described in https://github.com/ARM-software/abi-aa/blob/main/pauthabielf64/pauthabielf64.rst#relocation-compression.
This re-applies #87635 after the issue described in #87635 (comment) is fixed in ebc123e. A corresponding test is also added. Original PR description below. Support `R_AARCH64_AUTH_RELATIVE` relocation compression as described in https://github.com/ARM-software/abi-aa/blob/main/pauthabielf64/pauthabielf64.rst#relocation-compression.
This re-applies llvm#87635 after the issue described in llvm#87635 (comment) is fixed in ebc123e. A corresponding test is also added. Original PR description below. Support `R_AARCH64_AUTH_RELATIVE` relocation compression as described in https://github.com/ARM-software/abi-aa/blob/main/pauthabielf64/pauthabielf64.rst#relocation-compression.
Support
R_AARCH64_AUTH_RELATIVErelocation compression as described in https://github.com/ARM-software/abi-aa/blob/main/pauthabielf64/pauthabielf64.rst#relocation-compression