[PATCH 3/6] bfd: refine handling of relocations between debugging sections

[Jan Beulich]

On 05.03.2021 13:00, Alan Modra wrote:
> On Thu, Mar 04, 2021 at 10:00:58AM +0100, Jan Beulich wrote:
>> On 04.03.2021 07:10, Alan Modra wrote:
>>> So this is for linking ELF with debug info into PE output?
>>
>> It's unavoidable for this case, yes, but as said even for ELF it can
>> turn out to be necessary (when the .debug_* sections get assigned a
>> non-zero [and large enough] VA).
> 
> Most ELF targets won't use bfd_perform_relocation when linking native
> objects, so any hacks here won't help.

I've extended the patch description accordingly. Thanks for pointing
this out to me. Looks like to get ELF working this may then require
in some cases a per-target fix. Afaics a change might be possible in
a somewhat centralized manner in _bfd_elf_rela_local_sym(), but
targets using _bfd_elf_rel_local_sym() would need dealing with
individually. I've not looked into what it would take to deal with
relocations targeting global symbols, both because for debug info
this shouldn't matter and because I'm not after fixing ELF output
anyway.

>>>> --- a/bfd/reloc.c
>>>> +++ b/bfd/reloc.c
>>>> @@ -749,6 +749,30 @@ bfd_perform_relocation (bfd *abfd,
>>>>    else
>>>>      output_base = reloc_target_output_section->vma;
>>>>  
>>>> +  /* Most architectures have no section relative ELF relocations.  They use
>>>> +     ordinary ones instead for representing section relative references between
>>>> +     debugging sections, which works fine as long as the section VMA gets set
>>>> +     to zero.  While this is the default for ELF output (albeit not a
>>>> +     requirement), in particular PE doesn't even allow zero VMAs for any of the
>>>> +     sections.  */
>>>> +  if(output_base && !howto->pc_relative
>>>> +     && bfd_get_flavour (abfd) == bfd_target_elf_flavour
>>>> +     && (reloc_target_output_section->flags
>>>> +	 & input_section->flags & SEC_DEBUGGING))
>>>> +    {
>>>> +      /* Since this is a heuristic, apply further checks in an attempt to
>>>> +	 exclude relocation types other than simple base ones.  */
>>>> +      unsigned int size = bfd_get_reloc_size (howto);
>>>> +
>>>> +      if (size && !(size & (size - 1))
>>>> +          && !(howto->bitsize & (howto->bitsize - 1))
>>>> +          && !howto->bitpos && !howto->rightshift
>>>> +          && !howto->negate && !howto->partial_inplace
>>>> +          && !(howto->src_mask & (howto->src_mask + 1))
>>>> +          && !(howto->dst_mask & (howto->dst_mask + 1)))
>>>> +	output_base = 0;
>>>> +    }
>>>> +
>>>>    output_base += symbol->section->output_offset;
>>>>  
>>>>    /* If symbol addresses are in octets, convert to bytes.  */
>>>
>>> When we need this sort of horrible hack, it's time to redesign.
>>
>> Well, I was fearing that, but I have to admit I have no idea what
>> would need doing where. Surely the "horrible" aspects could be
>> reduced by limiting the number of extra checks - as said in
>> comment and description, I've added them to reduce risk of
>> mistakenly zapping output_base. But I definitely agree that no
>> matter how much massaging would be done, it's probably going to
>> remain ugly without finding an entirely different approach.
> 
> Yes, it's true all the extra conditions don't inspire confidence.  If
> it's OK to do at all then it should be OK with just
> 
>   if (output_bfd == NULL
>       && bfd_get_flavour (abfd) == bfd_target_elf_flavour
>       && (input_section->flags & SEC_DEBUGGING) != 0
>       && (symbol->section->flags & SEC_DEBUGGING) != 0)
>     output_base = 0;
> 
> with a comment saying why, of course.

While, as said, I agree the extra conditions don't look reassuring,
dropping them adds to what we imply towards relocations between
debugging sections. I understand you're okay with that (as far as
"okay" can go with a hack like this)?

With the code snippet above I observe a few more modifications
than just to drop the extra conditions. You use symbol->section
when I used reloc_target_output_section. Is that for a reason?

You've also split my

     && (reloc_target_output_section->flags
	 & input_section->flags & SEC_DEBUGGING))

which was probably just a style change (including the addition of
() != 0 around them). While I don't really see the reason for
this transformation, if that's what binutils style requires I can
surely change. However, meanwhile I've realized that
input_section's flags shouldn't really matter: If there indeed
was a relocation in a non-debugging section targeting a debugging
one, I would suppose output_base should be similarly ignored.
Thoughts? (Such relocations don't seem to make a lot of sense, so
perhaps this aspect is largely academical anyway.)

>  I can't say I like it though,
> and I'm inclined to say the right approach is to fix the bad DWARF.

I wonder what you mean by "fix the bad Dwarf": It's the lack of
suitable relocations in most targets' ELF ABIs which makes this
hack necessary. Are you suggesting all targets supporting Dwarf
should gain new section-relative relocation types?

I ask because - as indicated before - I'd be more than happy to
use a less hacky approach. Going the new-reloc-types route would
be a rather long process though, whereas the desire to not need
to discard debug info when linking EFI executables is a present
one.

Jan