diff mbox series

[v2,1/1] Fix IAT (Import Address Table) alignment on AArch64

Message ID 20260415080521.84647-2-evgeny.karpov@arm.com
State Superseded
Headers
Series Fix IAT (Import Address Table) alignment on AArch64 |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Evgeny Karpov April 15, 2026, 8:05 a.m. UTC 
  The IAT should be aligned to 8 bytes in aarch64-w64-mingw32, otherwise, it might
result in relocation issues.

When a function is imported from DLL, it generates the following code:

adrp    x19, __imp_fn
ldr     x19, [x19, #:lo12:__imp_fn]

8 byte alignment is required for ldr relocation. Addresses are placed in IAT.
The size of the chunk on AArch64 is 8 bytes.
If IAT is not aligned to 8 bytes, the relocation issue appears.
This patch fixes this issue by aligning IAT to 8 bytes on AArch64.

binutils/ChangeLog:

	* dlltool.c (make_head): Add 8 byte alignment on AArch64.
---
 binutils/dlltool.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Jan Beulich April 17, 2026, 6:29 a.m. UTC | #1 
On 15.04.2026 10:05, Evgeny Karpov wrote:
> --- a/binutils/dlltool.c
> +++ b/binutils/dlltool.c
> @@ -2735,6 +2735,12 @@ make_head (void)
>    if (!no_idata5)
>      {
>        fprintf (f, "\t.section\t.idata$5\n");
> +
> +      /* Align IAT (Import Address Table) to 8 bytes on AArch64.
> +	 https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#import-address-table.  */
> +      if (machine == MAARCH64)
> +	fprintf (f, "\t.p2align 3\n");

Hmm, so we're back to square 1 as far as the actual code change is concerned.
Further, I don't see how the link is related here. As you said yourself in a
reply on the v1 thread, there's no talk of alignment there. Neither your text
nor what is said there really addresses what I asked for in [1].

I also continue to be unhappy about the adhoc, hard-coded 3 in here. As
indicated, imo the function would better consult secdata_plain[], with that
in turn getting edited suitably (either at runtime, or by splitting the
"align" field into a pair of fields, one for PE32 and the other for PE32+).
The tool as a whole really wants to be self-consistent.

Yet further, as also indicated already, I don't follow why .idata$4 wouldn't
need (or at least want) treating the same. It also contains 8-byte elements
in PE32+.

Finally you still leave make_delay_head() entirely alone, when - as
indicated before as well - it clearly has the same issue.

IOW - I, for one, am not going to approve the change in its present shape.
I could make a patch (or perhaps rather a pair of patches) myself along the
lines of the above, if need be.

Jan

[1] https://sourceware.org/pipermail/binutils/2026-April/148874.html
Evgeny Karpov May 5, 2026, 8:14 a.m. UTC | #2 
On Fri, Apr 17, 2026, Jan Beulich wrote:
> Hmm, so we're back to square 1 as far as the actual code change is concerned.
> Further, I don't see how the link is related here. As you said yourself in a
> reply on the v1 thread, there's no talk of alignment there. Neither your text
> nor what is said there really addresses what I asked for in [1].

v3 will contain more generic changes to apply alignment to the IAT for PE32+.

> I also continue to be unhappy about the adhoc, hard-coded 3 in here. As
> indicated, imo the function would better consult secdata_plain[], with that
> in turn getting edited suitably (either at runtime, or by splitting the
> "align" field into a pair of fields, one for PE32 and the other for PE32+).
> The tool as a whole really wants to be self-consistent.
 
secdata*.align will be adjusted for PE32+.

> Yet further, as also indicated already, I don't follow why .idata$4 wouldn't
> need (or at least want) treating the same. It also contains 8-byte elements
> in PE32+.

.idata$4 emits the Import Lookup Table, which was not the purpose of this patch to resolve
issues with relocating symbols from IAT when the function is called.
 
> Finally you still leave make_delay_head() entirely alone, when - as
> indicated before as well - it clearly has the same issue.

v3 will contain changes in make_delay_head.

Regards,
Evgeny
Jan Beulich May 5, 2026, 8:58 a.m. UTC | #3 
On 05.05.2026 10:14, Evgeny Karpov wrote:
> On Fri, Apr 17, 2026, Jan Beulich wrote:
>> Hmm, so we're back to square 1 as far as the actual code change is concerned.
>> Further, I don't see how the link is related here. As you said yourself in a
>> reply on the v1 thread, there's no talk of alignment there. Neither your text
>> nor what is said there really addresses what I asked for in [1].
> 
> v3 will contain more generic changes to apply alignment to the IAT for PE32+.
> 
>> I also continue to be unhappy about the adhoc, hard-coded 3 in here. As
>> indicated, imo the function would better consult secdata_plain[], with that
>> in turn getting edited suitably (either at runtime, or by splitting the
>> "align" field into a pair of fields, one for PE32 and the other for PE32+).
>> The tool as a whole really wants to be self-consistent.
>  
> secdata*.align will be adjusted for PE32+.
> 
>> Yet further, as also indicated already, I don't follow why .idata$4 wouldn't
>> need (or at least want) treating the same. It also contains 8-byte elements
>> in PE32+.
> 
> .idata$4 emits the Import Lookup Table, which was not the purpose of this patch to resolve
> issues with relocating symbols from IAT when the function is called.
>  
>> Finally you still leave make_delay_head() entirely alone, when - as
>> indicated before as well - it clearly has the same issue.
> 
> v3 will contain changes in make_delay_head.

Btw, may I ask that you configure your mail program so that it properly
identifies replies? Just now I had "[PATCH v2 1/1] Fix IAT (Import
Address Table) alignment on AArch64" and "[PATCH v3 1/1] Fix IAT (Import
Address Table) alignment on AArch64" in my inbox. Clearly indicating
that the latter was an almost instant re-submission of the former (when
really the former was a reply on an old thread). In such an event I
would simply delete the obviously obsolete version, without even opening
it. It was only when looking at v3 0/1 that I recalled that your emails
lack the usual "Re:" prefix (or whichever translated version thereof).

Jan
diff mbox series

Patch

diff --git a/binutils/dlltool.c b/binutils/dlltool.c
index 94805fcd334..44823e120af 100644
--- a/binutils/dlltool.c
+++ b/binutils/dlltool.c
@@ -2735,6 +2735,12 @@  make_head (void)
   if (!no_idata5)
     {
       fprintf (f, "\t.section\t.idata$5\n");
+
+      /* Align IAT (Import Address Table) to 8 bytes on AArch64.
+	 https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#import-address-table.  */
+      if (machine == MAARCH64)
+	fprintf (f, "\t.p2align 3\n");
+
       if (use_nul_prefixed_import_tables)
 	{
 	  if (create_for_pep)