[PATCH v4] elf/x86-64: Subtract __ImageBase for R_AMD64_IMAGEBASE

[H.J. Lu]

On Thu, Mar 4, 2021 at 5:14 AM Alan Modra <amodra@gmail.com> wrote:
>
> On Mon, Mar 01, 2021 at 05:44:48AM -0800, H.J. Lu wrote:
> > On Mon, Mar 1, 2021 at 4:19 AM Alan Modra <amodra@gmail.com> wrote:
> > >
> > > On Sat, Feb 27, 2021 at 06:38:24PM -0800, H.J. Lu wrote:
> > > > On Wed, Feb 24, 2021 at 6:09 PM Alan Modra <amodra@gmail.com> wrote:
> > > > >
> > > > > On Tue, Feb 23, 2021 at 09:50:20AM -0800, H.J. Lu via Binutils wrote:
> > > > > >       * reloc.c (bfd_perform_relocation): Add a link_info argument.
> > > > > >       Subtract __ImageBase for R_AMD64_IMAGEBASE.
> > > > >
> > > > > H.J., did you consider moving all of this, including the code you
> > > > > added in git commit 36068e2fa54, to the relevant howto special
> > > > > functions?  None of it really belongs in bfd_perform_relocation.
> > > >
> > > > The howto special function, coff_amd64_reloc, isn't changed since
> > > >
> > > > 1. Any changes to coff_amd64_reloc may break Windows x86-64 linker.
> > >
> > > I am sure you are capable of making those changes without breaking
> > > windows.
> >
> > The difficult part is I can't test it.
>
> Which says that you also can't test your bfd_perform_relocation
> changes on windows.
>
> > > > 2. bfd_perform_relocation changes are safe since they are limited to
> > > > generating x86-64 ELF executable from Windows x86-64 relocatable object
> > > > files.
> > >
> > > It is the wrong place.  Every time bfd_perform_relocation is called
> > > with a coff input it now needs to test for output to ELF.  If you did
> > > this properly in a howto function then the extra tests would only be
> >
> > bfd_perform_relocation is called with
> >
> >            r = bfd_perform_relocation (input_bfd,
> >                                         *parent,
> >                                         data,
> >                                         input_section,
> >                                         relocatable ? abfd : NULL,
> > <<< output_bfd
> >                                         &error_message);
> >
> > and coff_amd64_reloc is very hard to understand.
>
> Yes, it surely is.  You do realize that coff_amd64_reloc has already
> made changes to the section contents before you again make changes in
> bfd_perform_relocation.  Arguing that it is easier to support ELF
> output in bfd_perform_relocation doesn't make much sense.  To do it
> correctly there you need to understand coff_amd64_reloc anyway.
>
> Are your bfd_perform_relocation changes correct for every x86_64 PE
> reloc, and for weak and common symbols?
>
> >
> > > done for x86_64 coff.  Besides the performance impact, nobody
> > > maintaining reloc.c wants to deal with code specially for pe-x86-64
> > > and elf64-x84-64.  The special case for coff there is already
> > > horrible.  It shouldn't be made worse.
> >
> >  I made the change in bfd_perform_relocation since the COFF
> > linker is very delicate.
> >
> > > Did anyone review your commit 36068e2fa54?
> >
> > No.  My change is limited to a very special case of ELF/x86-64.
> >
> > bfd_perform_relocation has
> >
> >       /* WTF?? */
> >       if (abfd->xvec->flavour == bfd_target_coff_flavour
> >           && strcmp (abfd->xvec->name, "coff-Intel-little") != 0
> >           && strcmp (abfd->xvec->name, "coff-Intel-big") != 0)
> >         {
> >
> > with a long comment.   I added:
>
> Right, and the "WTF??" is exclaiming that this sort of thing should
> not be done in bfd_perform_relocation.  The long comment is trying to
> explain why it is there anyway, and reinforcing what I'm saying says
> this: "The first obvious point is that bfd_perform_relocation should
> not have any tests that depend upon the flavour."
>
> I'm going to revert your bfd_perform_relocation changes, and possibly
> apply a much simpler coff_amd64_reloc change that happens to make
> things work for the testcases you added.  I have no confidence that
> the coff_amd64_reloc patch is correct in all cases, but it looks more
> reasonable to me.  I'd appreciate you checking it.
>
>         * reloc.c (bfd_perform_relocation): Revert 2021-01-12 and
>         2020-09-16 changes.
>         * coff-x86_64.c (coff_amd64_reloc): Do more or less the same
>         adjustments here instead.  Tidy comments and formatting.
>

Here is the v4 patch on top of yours.

Thanks.

-- 
H.J.