Re: [PATCH] PR30437 aarch64: make RELA relocs idempotent

[Michael Matz <matz@suse.de>]

Hello,

On Wed, 24 May 2023, Richard Earnshaw (lists) wrote:

> > Tested for aarch64-elf and aarch64-linux (and the other ~150 targets,
> > though those can't be affected).  Also tested natively on
> > aarch64-suse-linux-gnu.  Okay for master?
> > 
> 
> Isn't this what partial_inplace is supposed to describe?  Since that's 
> false, I'd expect any value stored in the section contents to be 
> ignored.

One might think so, yes.  But in practice that doesn't seem to be the case 
(anymore?).  I can't quite wrap my head around the logic in reloc.c and 
interaction between partial_inplace and src_mask.  The documentation says 
that partial_inplace _and_ src_mask should be zero for RELA targets (or 
rather, that other values are suspicious), and obviously having the 
former, but not the latter does make a difference.  (FWIW, x86_64 recently 
was fixed as well to have zero src_mask, and most other RELA targets also 
have it zero)

It's possible that partial_inplace only affects partial links (ld -r), at 
least the docu for that field specifically talks about only that, whereas 
here (the go bug) we have the problem of a final link where the section 
contents are non-zero.  I don't know if partial_inplace should or should 
not also affect final links, and if it's a bug that src_mask matters even 
with !partial_replace.  But one of the two needs to yield: either the 
logic regarding partial_inplace in reloc.c (so that src_mask isn't 
relevant anymore), or src_mask needs to be zero.  The advantage of the 
latter change is that it's aarch64-specific, whereas a change to 
partial_inplace handling affects everything and needs more bravery than I 
have.

Either way, even _if_ partial_inplace would be made so that src_mask 
doesn't matter anymore, then the change to zero src_mask would then still 
be at least harmless.

(skimming some more uses of src_mask relative to uses of partial_inplace 
in the aarch64 target also shows some more possibly dubious consideration 
of original section contents in _bfd_aarch64_elf_put_addend.  It always 
looks at the original content and uses src_mask as selector of being for 
insns or data, but then goes on to mask the read contents only against 
dst_mask, and not against src_mask.  I don't quite know what to make of 
that) 


Ciao,
Michael.