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

[Alan Modra]

On Mon, Mar 08, 2021 at 03:17:54PM +0100, Jan Beulich wrote:
> 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.

Yes, I deliberatey changed the test of what is effectively
  symbol->section->output_section->flags
to
  symbol->section->flags

You care about what is in the input section, not the output section.
The output section may even be .data, for example.

(As far as the style issue goes, I wasn't unhappy with the way you
wrote the expression.  The change was mostly to avoid a line-wrap in a
place you wouldn't normally wrap if allowed very long lines.)

> 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.)

Yes, it likely is academic, but you put forward the idea of loaded
DWARF data accessed by an application.  In that situation you want
relocations from non-DWARF to DWARF to behave normally: The DWARF is
just a chunk of data.

> >  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?

It certainly would be nice if the correct relocations were available,
and used.  That would be taking a very hard line though.  I was
wondering if we can correct the DWARF relocations in the linker,
before we get to bfd_perform_relocation.  None of the ideas I had work
very well though, for example trying to do something in
elf_slurp_reloc_table_from_section to set a howto flag asking for
section-relative values does minimize changes in
bfd_perform_relocation, but you'd need all the howtos to be writable
or be switching howtos in elf_slurp_reloc_table_from_section.
Besides, adding a flag requires lots of editing for little real
benefit over accepting another hack in bfd_perform_relocation.


> 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

Does this work for you?

	* elf.c (bfd_elf_generic_reloc): Make references between debug
	sections use section relative values.

diff --git a/bfd/elf.c b/bfd/elf.c
index 553fa65a11..5331a7064a 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -1302,8 +1302,7 @@ const char *const bfd_elf_section_type_names[] =
    change anything about the way the reloc is handled, since it will
    all be done at final link time.  Rather than put special case code
    into bfd_perform_relocation, all the reloc types use this howto
-   function.  It just short circuits the reloc if producing
-   relocatable output against an external symbol.  */
+   function, or should call this function for relocatable output.  */
 
 bfd_reloc_status_type
 bfd_elf_generic_reloc (bfd *abfd ATTRIBUTE_UNUSED,
@@ -1323,6 +1322,19 @@ bfd_elf_generic_reloc (bfd *abfd ATTRIBUTE_UNUSED,
       return bfd_reloc_ok;
     }
 
+  /* In some cases the relocation should be treated as input section
+     relative, as when linking ELF DWARF into PE COFF.  Many ELF
+     targets lack section relative relocations and instead use
+     ordinary absolute relocations for references between DWARF
+     sections.  That is arguably a bug in those targets but it happens
+     to work for the usual case of linking to non-loaded ELF debug
+     sections with VMAs forced to zero.  PE COFF on the other hand
+     doesn't allow a section VMA of zero.  */
+  if (output_bfd == NULL
+      && (symbol->section->flags & SEC_DEBUGGING) != 0
+      && (input_section->flags & SEC_DEBUGGING) != 0)
+    reloc_entry->addend -= symbol->section->output_section->vma;
+
   return bfd_reloc_continue;
 }
 


-- 
Alan Modra
Australia Development Lab, IBM