diff mbox series

aarch64-pe can't fill 16 bytes in section .text

Message ID Y3QKUwDn748CbDIs@squeak.grove.modra.org
State Repeat Merge
Headers
Series aarch64-pe can't fill 16 bytes in section .text |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

Alan Modra Nov. 15, 2022, 9:53 p.m. UTC 
  I'm going to fix the underlying coff_frob_section problem too.
Without commit b66e671854, this:
 .p2align 4
 nop
 .p2align 3
 nop
results in an error when coff_frob_section attempts to pad out the
section to a 16-byte boundary.  Due to miscalculating the pad pattern
repeat count, write.c:write_contents attempts to shove 16 bytes of
padding into the remaining 4 bytes of the .text section.

	* config/obj-coff.c (coff_frob_section): Correct fill count.
	Don't pad after errors.

Comments

Zac Walker Nov. 16, 2022, 5:03 p.m. UTC | #1 
Thanks Alan,

Do you think my SUB_SEGMENT_ALIGN patch is still needed. I didn't merge it yet because I wanted to do more testing with your fix.

Zac

---
  gas/config/tc-aarch64.h | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
index 2d514ff610..0ea73021f2 100644
--- a/gas/config/tc-aarch64.h
+++ b/gas/config/tc-aarch64.h
@@ -191,7 +191,10 @@ struct aarch64_frag_type
        goto LABEL;								\
      }

+/* COFF sub section alignment calculated using the write.c implementation.  */
+#ifndef OBJ_COFF
  #define SUB_SEGMENT_ALIGN(SEG, FRCHAIN) 0
+#endif

  #define DWARF2_LINE_MIN_INSN_LENGTH 	4
Alan Modra Nov. 17, 2022, 3:01 a.m. UTC | #2 
On Wed, Nov 16, 2022 at 06:03:56PM +0100, Zac Walker wrote:
> Thanks Alan,
> 
> Do you think my SUB_SEGMENT_ALIGN patch is still needed. I didn't merge it yet because I wanted to do more testing with your fix.

Huh, I thought it had already gone in, but it was just in my local
tree.

Your patch isn't needed with my fix to obj-coff.c, but it wouldn't be
a bad idea to apply it anyway.  We have a few too many places doing
section padding.  It would be nice to get rid of the alignment code
in coff_frob_section.
Frager, Neal via Binutils Nov. 18, 2022, 8:52 a.m. UTC | #3 
Patch merged just now. (Was a holiday here yesterday)

I am still investigating another alignment problem so might be another follow up patch. This code from OpenBLAS:

data_ar:
 .word 0x3e44fae6
data_ai:
 .word 0x3d320fa2
start:
 ldr s20, data_ar
 ldr s21, data_ai 

Produces:

zscal-min.s:2: Warning: value 0x3e44fae6 truncated to 0xfae6
zscal-min.s:4: Warning: value 0x3d320fa2 truncated to 0xfa2
zscal-min.s:7: Error: pc-relative load offset not word aligned

Works ok with clang. I am not sure if .word data should be aligned by default in gas or this is just a difference in behaviour.

Thanks,
Zac

-----Original Message-----
From: Alan Modra <amodra@gmail.com> 
Sent: 17 November 2022 04:02
To: Zac Walker <zac.walker@linaro.org>
Cc: Nick Clifton <nickc@redhat.com>; binutils@sourceware.org
Subject: Re: aarch64-pe can't fill 16 bytes in section .text

On Wed, Nov 16, 2022 at 06:03:56PM +0100, Zac Walker wrote:
> Thanks Alan,
> 
> Do you think my SUB_SEGMENT_ALIGN patch is still needed. I didn't merge it yet because I wanted to do more testing with your fix.

Huh, I thought it had already gone in, but it was just in my local tree.

Your patch isn't needed with my fix to obj-coff.c, but it wouldn't be a bad idea to apply it anyway.  We have a few too many places
doing section padding.  It would be nice to get rid of the alignment code in coff_frob_section.

--
Alan Modra
Australia Development Lab, IBM
Jan Beulich Nov. 18, 2022, 9:08 a.m. UTC | #4 
On 18.11.2022 09:52, Zac Walker via Binutils wrote:
> Patch merged just now. (Was a holiday here yesterday)
> 
> I am still investigating another alignment problem so might be another follow up patch. This code from OpenBLAS:
> 
> data_ar:
>  .word 0x3e44fae6
> data_ai:
>  .word 0x3d320fa2
> start:
>  ldr s20, data_ar
>  ldr s21, data_ai 
> 
> Produces:
> 
> zscal-min.s:2: Warning: value 0x3e44fae6 truncated to 0xfae6
> zscal-min.s:4: Warning: value 0x3d320fa2 truncated to 0xfa2
> zscal-min.s:7: Error: pc-relative load offset not word aligned
> 
> Works ok with clang. I am not sure if .word data should be aligned by default in gas or this is just a difference in behaviour.

.word is overridden by tc-aarch64.c only for ELF. So in your COFF (aiui)
case .word emits just two bytes instead of the expected four. Hence also
the warnings, not just the error.

Jan
Frager, Neal via Binutils Nov. 18, 2022, 9:35 a.m. UTC | #5 
Thanks for the help Jan. Adding a similar override to "tc-arm.c" fixes it:

#ifdef OBJ_ELF
 ...
#else
  { "word", cons, 4},
#endif

I will do some testing on other pseudo types in case I am missing more. 

Zac

-----Original Message-----
From: Jan Beulich <jbeulich@suse.com> 
Sent: 18 November 2022 10:09
To: zac.walker@linaro.org
Cc: 'Nick Clifton' <nickc@redhat.com>; binutils@sourceware.org; 'Alan Modra' <amodra@gmail.com>
Subject: Re: aarch64-pe can't fill 16 bytes in section .text

On 18.11.2022 09:52, Zac Walker via Binutils wrote:
> Patch merged just now. (Was a holiday here yesterday)
> 
> I am still investigating another alignment problem so might be another follow up patch. This code from OpenBLAS:
> 
> data_ar:
>  .word 0x3e44fae6
> data_ai:
>  .word 0x3d320fa2
> start:
>  ldr s20, data_ar
>  ldr s21, data_ai
> 
> Produces:
> 
> zscal-min.s:2: Warning: value 0x3e44fae6 truncated to 0xfae6
> zscal-min.s:4: Warning: value 0x3d320fa2 truncated to 0xfa2
> zscal-min.s:7: Error: pc-relative load offset not word aligned
> 
> Works ok with clang. I am not sure if .word data should be aligned by default in gas or this is just a difference in behaviour.

.word is overridden by tc-aarch64.c only for ELF. So in your COFF (aiui) case .word emits just two bytes instead of the expected four. Hence also the warnings, not just the error.

Jan
diff mbox series

Patch

diff --git a/gas/config/obj-coff.c b/gas/config/obj-coff.c
index 98c39e43907..9be697fb62e 100644
--- a/gas/config/obj-coff.c
+++ b/gas/config/obj-coff.c
@@ -1725,7 +1725,8 @@  coff_frob_section (segT sec)
   bfd_vma align_power = (bfd_vma) sec->alignment_power + OCTETS_PER_BYTE_POWER;
   bfd_vma mask = ((bfd_vma) 1 << align_power) - 1;
 
-  if (size & mask)
+  if (!do_not_pad_sections_to_alignment
+      && (size & mask) != 0)
     {
       bfd_vma new_size;
       fragS *last;
@@ -1740,7 +1741,10 @@  coff_frob_section (segT sec)
       while (fragp->fr_next != last)
 	fragp = fragp->fr_next;
       last->fr_address = size;
-      fragp->fr_offset += new_size - size;
+      if ((new_size - size) % fragp->fr_var == 0)
+	fragp->fr_offset += (new_size - size) / fragp->fr_var;
+      else
+	abort ();
     }
 #endif