diff mbox series

[1/5] Fix size of external_reloc for pe-aarch64

Message ID 20221216021400.22309-1-mark@harmstone.com
State Accepted
Headers
Series [1/5] Fix size of external_reloc for pe-aarch64 |

Checks

Context Check Description
snail/binutils-gdb-check success Github commit url

Commit Message

Mark Harmstone Dec. 16, 2022, 2:13 a.m. UTC 
  This patch series finishes off the work by Jedidiah Thompson, and adds
support for creating aarch64 PE images.

This should be essentially complete: I've used this to create a "hello
world" Windows program in asm, and (with GCC patches) a UEFI program in
C. I think the only things missing are the .secidx relocation, which is
needed for PDBs, and the SEH pseudos used for C++ exceptions.

This first patch fixes the size of RELSZ; I'm not sure why it was 14 in
the first place. This is the size of the "Base Relocation Block" in
https://learn.microsoft.com/en-us/windows/win32/debug/pe-format, and
AFAIK should be 10 for everything.

---
 bfd/coff-aarch64.c     | 4 ----
 include/coff/aarch64.h | 3 +--
 2 files changed, 1 insertion(+), 6 deletions(-)

Comments

Jan Beulich Dec. 16, 2022, 7:03 a.m. UTC | #1 
On 16.12.2022 03:13, Mark Harmstone wrote:
> This patch series finishes off the work by Jedidiah Thompson, and adds
> support for creating aarch64 PE images.
> 
> This should be essentially complete: I've used this to create a "hello
> world" Windows program in asm, and (with GCC patches) a UEFI program in
> C. I think the only things missing are the .secidx relocation, which is
> needed for PDBs, and the SEH pseudos used for C++ exceptions.
> 
> This first patch fixes the size of RELSZ; I'm not sure why it was 14 in
> the first place. This is the size of the "Base Relocation Block" in
> https://learn.microsoft.com/en-us/windows/win32/debug/pe-format, and
> AFAIK should be 10 for everything.

Not sure - there look to be different formats in use, judging from other
headers in include/coff/. See e.g. arm.h which even has two forms.
Clearly in the original commit (b69c9d41e894), targeting only objcopy
(and maybe objdump) for PE binaries, this didn't really matter. Does it
actually matter for you, when you're also only targeting PE? Or are you
(in spite of the title) really after COFF objects as well?

Let me Cc Tamar, the author of the original patch, to possibly shed
some more light on what the struct was initially derived from.

Jan
Tamar Christina Dec. 16, 2022, 10:47 a.m. UTC | #2 
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, December 16, 2022 7:04 AM
> To: Mark Harmstone <mark@harmstone.com>
> Cc: binutils@sourceware.org; wej22007@outlook.com;
> zac.walker@linaro.org; Tamar Christina <Tamar.Christina@arm.com>
> Subject: Re: [PATCH 1/5] Fix size of external_reloc for pe-aarch64
> 
> On 16.12.2022 03:13, Mark Harmstone wrote:
> > This patch series finishes off the work by Jedidiah Thompson, and adds
> > support for creating aarch64 PE images.
> >
> > This should be essentially complete: I've used this to create a "hello
> > world" Windows program in asm, and (with GCC patches) a UEFI program
> > in C. I think the only things missing are the .secidx relocation,
> > which is needed for PDBs, and the SEH pseudos used for C++ exceptions.
> >
> > This first patch fixes the size of RELSZ; I'm not sure why it was 14
> > in the first place. This is the size of the "Base Relocation Block" in
> > https://learn.microsoft.com/en-us/windows/win32/debug/pe-format, and
> > AFAIK should be 10 for everything.
> 
> Not sure - there look to be different formats in use, judging from other
> headers in include/coff/. See e.g. arm.h which even has two forms.
> Clearly in the original commit (b69c9d41e894), targeting only objcopy (and
> maybe objdump) for PE binaries, this didn't really matter. Does it actually
> matter for you, when you're also only targeting PE? Or are you (in spite of
> the title) really after COFF objects as well?
> 
> Let me Cc Tamar, the author of the original patch, to possibly shed some
> more light on what the struct was initially derived from.

The code was taken from the Arm port which weirdly had a different size
for this whether it was a WINCE target or not.  For WINCE target it has 10, but for
everything else 14 (See include/coff/arm.h).   Since I didn't have relocations
support anyway (the original intention was only to support objcopy workflow)
I didn't dig too deeply into why Arm has it as 14 and the extra field.

That said 10 looks to be the correct value.

Thanks,
Tamar

> 
> Jan
Mark Harmstone Dec. 20, 2022, 12:59 p.m. UTC | #3 
On 16/12/22 10:47, Tamar Christina wrote:
>
> The code was taken from the Arm port which weirdly had a different size
> for this whether it was a WINCE target or not.  For WINCE target it has 10, but for
> everything else 14 (See include/coff/arm.h).   Since I didn't have relocations
> support anyway (the original intention was only to support objcopy workflow)
> I didn't dig too deeply into why Arm has it as 14 and the extra field.
>
> That said 10 looks to be the correct value.
>
> Thanks,
> Tamar

Thanks Tamar. Sorry, I should have copied you in in the first place.

I can't remember the specifics, but I definitely needed this patch to get EXEs working on Windows.

Mark
Jan Beulich Dec. 20, 2022, 1:10 p.m. UTC | #4 
On 20.12.2022 13:59, Mark Harmstone wrote:
> On 16/12/22 10:47, Tamar Christina wrote:
>>
>> The code was taken from the Arm port which weirdly had a different size
>> for this whether it was a WINCE target or not.  For WINCE target it has 10, but for
>> everything else 14 (See include/coff/arm.h).   Since I didn't have relocations
>> support anyway (the original intention was only to support objcopy workflow)
>> I didn't dig too deeply into why Arm has it as 14 and the extra field.
>>
>> That said 10 looks to be the correct value.
>>
>> Thanks,
>> Tamar
> 
> Thanks Tamar. Sorry, I should have copied you in in the first place.
> 
> I can't remember the specifics, but I definitely needed this patch to get EXEs working on Windows.

But EXEs don't use this kind of relocation; the base relocations (fixups)
used there are encoded in an entirely different way. The relocs here should
matter for are COFF object files only.

Jan
Tamar Christina Dec. 20, 2022, 1:38 p.m. UTC | #5 
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, December 20, 2022 1:10 PM
> To: Mark Harmstone <mark@harmstone.com>
> Cc: binutils@sourceware.org; wej22007@outlook.com;
> zac.walker@linaro.org; Tamar Christina <Tamar.Christina@arm.com>
> Subject: Re: [PATCH 1/5] Fix size of external_reloc for pe-aarch64
> 
> On 20.12.2022 13:59, Mark Harmstone wrote:
> > On 16/12/22 10:47, Tamar Christina wrote:
> >>
> >> The code was taken from the Arm port which weirdly had a different
> >> size for this whether it was a WINCE target or not.  For WINCE target it has
> 10, but for
> >> everything else 14 (See include/coff/arm.h).   Since I didn't have
> relocations
> >> support anyway (the original intention was only to support objcopy
> >> workflow) I didn't dig too deeply into why Arm has it as 14 and the extra
> field.
> >>
> >> That said 10 looks to be the correct value.
> >>
> >> Thanks,
> >> Tamar
> >
> > Thanks Tamar. Sorry, I should have copied you in in the first place.
> >
> > I can't remember the specifics, but I definitely needed this patch to get
> EXEs working on Windows.
> 
> But EXEs don't use this kind of relocation; the base relocations (fixups) used
> there are encoded in an entirely different way. The relocs here should
> matter for are COFF object files only.

Agreed, the relocs here are COFF only.  That said 10 still looks like the correct value.
Microsoft's documentation seems to have become a bit unclear after they reworded it for ASLR support.
Originally the Image files didn't have relocations as by then they would have been resolved already, but for ASLR
there is exposed a .reloc section now so the loader can do the fixups.  That's what the docs now describe:
https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#the-reloc-section-image-only

They however no longer describe COFF relocations at all.  If we find an older spec (like Revision 6.0 - February 1999),
when it was still doc/pdf like https://courses.cs.washington.edu/courses/cse378/03wi/lectures/LinkerFiles/coff.pdf 
you'll see that the COFF relocations are described in section 5.2 are still 10 bytes long.

This also coincides with LLVM's description of the COFF relocations https://llvm.org/doxygen/structllvm_1_1COFF_1_1relocation.html

It's highly likely that there is a bug in the Arm port, because the most used PE target on Arm is WINCE which sets 10.

Cheers,
Tamar

> Jan
diff mbox series

Patch

diff --git a/bfd/coff-aarch64.c b/bfd/coff-aarch64.c
index 2c3e225a222..0faa75c63d2 100644
--- a/bfd/coff-aarch64.c
+++ b/bfd/coff-aarch64.c
@@ -188,10 +188,6 @@  coff_aarch64_rtype_lookup (unsigned int code)
 #define bfd_pe_print_pdata      NULL
 #endif
 
-/* Handle include/coff/aarch64.h external_reloc.  */
-#define SWAP_IN_RELOC_OFFSET	H_GET_32
-#define SWAP_OUT_RELOC_OFFSET	H_PUT_32
-
 /* Return TRUE if this relocation should
    appear in the output .reloc section.  */
 
diff --git a/include/coff/aarch64.h b/include/coff/aarch64.h
index 100e08f18ef..b670f28bd3e 100644
--- a/include/coff/aarch64.h
+++ b/include/coff/aarch64.h
@@ -54,11 +54,10 @@  struct external_reloc
   char r_vaddr[4];
   char r_symndx[4];
   char r_type[2];
-  char r_offset[4];
 };
 
 #define RELOC struct external_reloc
-#define RELSZ 14
+#define RELSZ 10
 
 /* ARM64 relocations types. */