[PATCH v3] elf/x86-64: Subtract __ImageBase for R_AMD64_IMAGEBASE
Alan Modra
amodra@gmail.com
Thu Mar 4 13:14:48 GMT 2021
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.
diff --git a/bfd/coff-x86_64.c b/bfd/coff-x86_64.c
index adab60cd11..63a7f06f9b 100644
--- a/bfd/coff-x86_64.c
+++ b/bfd/coff-x86_64.c
@@ -75,14 +75,14 @@ coff_amd64_reloc (bfd *abfd,
{
symvalue diff;
-#if !defined(COFF_WITH_PE)
+#if !defined (COFF_WITH_PE)
if (output_bfd == NULL)
return bfd_reloc_continue;
#endif
if (bfd_is_com_section (symbol->section))
{
-#if !defined(COFF_WITH_PE)
+#if !defined (COFF_WITH_PE)
/* We are relocating a common symbol. The current value in the
object file is ORIG + OFFSET, where ORIG is the value of the
common symbol as seen by the object file when it was compiled
@@ -113,14 +113,14 @@ coff_amd64_reloc (bfd *abfd,
/* Although PC relative relocations are very similar between
PE and non-PE formats, but they are off by 1 << howto->size
- bytes. For the external relocation, PE is very different
- from others. See md_apply_fix3 () in gas/config/tc-amd64.c.
+ bytes. For the external relocation, PE is very different
+ from others. See md_apply_fix in gas/config/tc-i386.c.
When we link PE and non-PE object files together to
generate a non-PE executable, we have to compensate it
here. */
- if(howto->pc_relative && howto->pcrel_offset)
+ if (howto->pc_relative && howto->pcrel_offset)
diff = -(1 << howto->size);
- else if(symbol->flags & BSF_WEAK)
+ else if (symbol->flags & BSF_WEAK)
diff = reloc_entry->addend - symbol->value;
else
diff = -reloc_entry->addend;
@@ -130,7 +130,20 @@ coff_amd64_reloc (bfd *abfd,
diff = reloc_entry->addend;
}
-#if defined(COFF_WITH_PE)
+#if defined (COFF_WITH_PE)
+ if (output_bfd == NULL
+ && (bfd_get_flavour (input_section->output_section->owner)
+ == bfd_target_elf_flavour))
+ {
+ if (reloc_entry->howto->pc_relative)
+ {
+ diff -= reloc_entry->addend;
+ if (reloc_entry->howto->type >= R_AMD64_PCRLONG_1
+ && reloc_entry->howto->type <= R_AMD64_PCRLONG_5)
+ diff -= (bfd_vma) (reloc_entry->howto->type - R_AMD64_PCRLONG);
+ }
+ }
+
/* FIXME: How should this case be handled? */
if (reloc_entry->howto->type == R_AMD64_IMAGEBASE
&& output_bfd != NULL
diff --git a/bfd/reloc.c b/bfd/reloc.c
index a7547187eb..5ed7bb8e59 100644
--- a/bfd/reloc.c
+++ b/bfd/reloc.c
@@ -51,7 +51,7 @@ SECTION
#include "bfdlink.h"
#include "libbfd.h"
#include "bfdver.h"
-#include "coff/x86_64.h"
+
/*
DOCDD
INODE
@@ -905,30 +905,6 @@ space consuming. For each target:
}
}
}
- else if (abfd->xvec->flavour == bfd_target_coff_flavour
- && (input_section->output_section->owner->xvec->flavour
- == bfd_target_elf_flavour)
- && strcmp (abfd->xvec->name, "pe-x86-64") == 0
- && strcmp (input_section->output_section->owner->xvec->name,
- "elf64-x86-64") == 0)
- {
- /* NB: bfd_perform_relocation isn't called to generate PE binary.
- _bfd_relocate_contents is called instead. When linking PE
- object files to generate ELF output, _bfd_relocate_contents
- isn't called and bfd_perform_relocation is used. We need to
- adjust relocation here. */
- relocation -= reloc_entry->addend;
- if (howto->type >= R_AMD64_PCRLONG_1
- && howto->type <= R_AMD64_PCRLONG_5)
- relocation -= (bfd_vma)(howto->type - R_AMD64_PCRLONG);
- else if (howto->type == R_AMD64_DIR64
- || howto->type == R_AMD64_DIR32)
- {
- bfd_vma val = read_reloc (abfd, (bfd_byte *) data + octets,
- howto);
- relocation -= val & howto->src_mask;
- }
- }
/* FIXME: This overflow checking is incomplete, because the value
might have overflowed before we get here. For a correct check we
--
Alan Modra
Australia Development Lab, IBM
More information about the Binutils
mailing list