inconsistencies if (at least) ELF relocation handling

[Alan Modra]

On Mon, Apr 19, 2021 at 11:05:19AM +0200, Jan Beulich via Binutils wrote:
> The comment next to struct reloc_howto_struct's partial_inplace field
> suggests that this is to be avoided by setting src_mask to zero.

Yes, the point of RELA relocations was to move addends out of the
section contents into the relocation.  So src_mask should generally be
zero for RELA.  The ELF gABI wording under "Relocation" supports this
too, talking about addends being "either from the field to be
relocated or from the addend field contained in the relocation
record".  It doesn't say "and/or".  However, when a target applies
multiple relocations to the same r_offset where the addend
accumulates, a target might accumulate such addends in the section
contents.  (Which isn't exactly correct, the accumulation ought to be
done at full word width not the width of the field.)  Such relocs
might need src_mask equal to dst_mask.

> can see that e.g. PPC, IA64, and RISC-V do so, but e.g. x86-64, Arm32,
> and Arm64 don't (I've looked at just the architectures that I'm at
> least remotely familiar with). Any thoughts about fixing this (Cc-ing
> the specific targets' maintainers for this reason)?

Correcting src_mask should be left to target maintainers.  The target
might have broken object files.

> However, even if src_mask was zero for all targets using only RELA
> relocations, there's then still an asymmetry in overflow checking:
> REL relocs, having their addends read out of section contents by e.g.
> _bfd_relocate_contents(), have overflow from adding in this addend
> properly checked. RELA relocs, having their addends added in already
> in e.g. _bfd_final_link_relocate(), don't. I've started putting
> together a patch (appended at the end of the mail), which is both
> incomplete (neither adjusting target-specific callers of
> _bfd_relocate_contents() yet, nor adjusting further callers of
> read_reloc() so far) and breaking at least x86-64 (see below). I'd
> still like to get input on what people think a proper approach would
> be here.

Yes, you're very likely going to expose a lot of target bugs if you
correct RELA overflow checking.  I tried something similar a long time
ago and decided the pain wasn't worth it.  Don't let me stop you
poking at it though, you've successfully taken on some tricky jobs
before.

> As to the breakage on x86-64, I observe a number of "relocation
> truncated to fit: R_X86_64_32 against `.debug_str'", due to negative
> addends getting truncated to 32-bit unsigned values and this reloc
> using complain_overflow_unsigned. In ELF it is my understanding that
> addends - no matter what their origin - are always signed.

The ELF gABI does tend to give that idea by specifying r_addend as
signed, but I believe there are relocations that only allow positive
addends.  Processor ABIs are free to specify relocations like that..

> For this
> case the present checking done for complain_overflow_unsigned looks
> wrong to me. But of course there may be object formats where unsigned
> addends might be used for at least some relocation types, so I'm
> hesitant to change the checking logic itself, and I'm instead
> wondering whether yet another complain_overflow_* type may be needed.
> Otoh there are only very few uses of the type outside of bfd/elf*.c.

-- 
Alan Modra
Australia Development Lab, IBM