Bug 12931 - ARM: gas fails to set the proper alignment on code sections, causing broken output
Summary: ARM: gas fails to set the proper alignment on code sections, causing broken o...
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gas (show other bugs)
Version: 2.22
: P2 normal
Target Milestone: ---
Assignee: unassigned
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-24 19:29 UTC by Dave Martin
Modified: 2014-05-28 19:42 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
possible fix (619 bytes, patch)
2011-06-28 12:57 UTC, Dave Martin
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Martin 2011-06-24 19:29:40 UTC
Whenever an instruction is assembled, gas should increase the alignment of the output section.

Currently the output section alignment is often left untouched, which may result in invalid alignment when sections are merged at link-time.

This appears to be responsible for a faulty WebM codec behaviour when firefox-5 is built in Thumb, as a result of ARM code being linked into .text alongside Thumb code.  The ARM instructions are placed at halfword alignment and so can't be executed correctly.

I don't know if this is a recent regression or an older bug.

It appears present in trunk as of 2011-06-24.
The bug is also present in linaro binutils 2.21.0.20110327-2ubuntu2cross1.62


In both of the cases below, the .text section alignment should not be 2**0.  The alignment should probably be set to 2**2 (though it could in principle be 2**1 for some Thumb code, some Thumb instructions are alignment-sensitive modulo 1 word; the 16-bit PC-relative add and ldr instructions have this restriction).

$ cat <<EOF >tst-align.s
.type f, %function
.globl f
f:      nop
EOF

$ arm-linux-gnueabi-as -o tst-align.o tst-align.s
$ arm-linux-gnueabi-objdump -hd tst-align.o

tst-align.o:     file format elf32-littlearm

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         00000004  00000000  00000000  00000034  2**0
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .data         00000000  00000000  00000000  00000038  2**0
                  CONTENTS, ALLOC, LOAD, DATA
  2 .bss          00000000  00000000  00000000  00000038  2**0
                  ALLOC
  3 .ARM.attributes 00000016  00000000  00000000  00000038  2**0
                  CONTENTS, READONLY

Disassembly of section .text:

00000000 <f>:
   0:   e1a00000        nop                     ; (mov r0, r0)
$ arm-linux-gnueabi-as mthumb  
$ arm-linux-gnueabi-objdump -hd tst-align.o

tst-align.o:     file format elf32-littlearm

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         00000002  00000000  00000000  00000034  2**0
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .data         00000000  00000000  00000000  00000036  2**0
                  CONTENTS, ALLOC, LOAD, DATA
  2 .bss          00000000  00000000  00000000  00000036  2**0
                  ALLOC
  3 .ARM.attributes 00000016  00000000  00000000  00000036  2**0
                  CONTENTS, READONLY

Disassembly of section .text:

00000000 <f>:
   0:   46c0            nop                     ; (mov r8, r8)
Comment 1 Dave Martin 2011-06-24 19:30:19 UTC
For details of the Firefox/WebM bug, see:

https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/789198
Comment 2 Dave Martin 2011-06-28 12:57:37 UTC
Created attachment 5824 [details]
possible fix

The attached patch hooks into mapping_state() to mark the alignment requirement on the current output section whenever we start emitting instructions.

It might be safer to make the alignment 4 for Thumb code as well as ARM code; the current patch gives Thumb code an alignment of 2 bytes, based on the principles that (a) this makes the current situation no worse, and (b) all realistic situations where a Thumb code section requires 4-byte alignment will involve an explicit .align or literal pool somewhere, so we don't need to worry.
Comment 3 Sourceware Commits 2011-06-29 16:29:43 UTC
CVSROOT:	/cvs/src
Module name:	src
Changes by:	nickc@sourceware.org	2011-06-29 16:29:38

Modified files:
	gas            : ChangeLog 
	gas/config     : tc-arm.c 

Log message:
	PR gas/12931
	* config/tc-arm.c (mapping_state): When changing to ARM or THUMB
	state set the minimum required alignment of the section.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/ChangeLog.diff?cvsroot=src&r1=1.4527&r2=1.4528
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/config/tc-arm.c.diff?cvsroot=src&r1=1.495&r2=1.496
Comment 4 Nick Clifton 2011-06-29 16:33:44 UTC
Hi Dave,

  Thanks for reporting this problem and providing a patch to fix it too.  I think that 2 byte alignment should be fine for thumb-code-containing sections, but if someone does come up with a scenario where this assumption does not work then we can always fix the assembler again.

Cheers
  Nick
Comment 5 Dave Martin 2011-06-29 16:47:08 UTC
On Wed, Jun 29, 2011 at 5:33 PM, nickc at redhat dot com
<sourceware-bugzilla@sourceware.org> wrote:
> http://sourceware.org/bugzilla/show_bug.cgi?id=12931
>
> Nick Clifton <nickc at redhat dot com> changed:
>
>           What    |Removed                     |Added
> ----------------------------------------------------------------------------
>             Status|NEW                         |RESOLVED
>                 CC|                            |nickc at redhat dot com
>         Resolution|                            |FIXED
>
> --- Comment #4 from Nick Clifton <nickc at redhat dot com> 2011-06-29 16:33:44 UTC ---
> Hi Dave,
>
>  Thanks for reporting this problem and providing a patch to fix it too.  I
> think that 2 byte alignment should be fine for thumb-code-containing sections,
> but if someone does come up with a scenario where this assumption does not work
> then we can always fix the assembler again.

That seems reasonable -- I think the proposed fix should be a useful
interim step, since it's pretty simple and makes the situation no
worse.

Did the patch look sensible to you?  I'm no gas hacker...


Cheers
---Dave
Comment 6 Nick Clifton 2011-06-29 16:53:42 UTC
Hi Dave,

> Did the patch look sensible to you?  I'm no gas hacker...

Are you sure ? ... The patch was fine. :-)

Cheers
   Nick
Comment 7 Sourceware Commits 2011-06-30 13:07:24 UTC
CVSROOT:	/cvs/src
Module name:	src
Changes by:	nickc@sourceware.org	2011-06-30 13:07:21

Modified files:
	gas/testsuite  : ChangeLog 
	gas/testsuite/gas/arm: blx-bad.d inst-po-be.d inst-po.d 

Log message:
	PR gas/12931
	* gas/arm/blx-bad.d: Add exrta nop at end of disassembly.
	* gas/arm/inst-po-be.d: Add exrta nop at end of disassembly.
	* gas/arm/inst-po.d: Add exrta nop at end of disassembly.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/ChangeLog.diff?cvsroot=src&r1=1.1925&r2=1.1926
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/arm/blx-bad.d.diff?cvsroot=src&r1=1.2&r2=1.3
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/arm/inst-po-be.d.diff?cvsroot=src&r1=1.1&r2=1.2
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/arm/inst-po.d.diff?cvsroot=src&r1=1.1&r2=1.2
Comment 8 Hans-Peter Nilsson 2011-07-01 04:19:31 UTC
Nick (being already CC:ed on the referred PR) or anyone else ARM-knowledgeable, pretty please have a quick look to see if the fix for this bug was the proper fix for PR sim/12737.  Thanks.
Comment 9 Jackie Rosen 2014-02-16 17:50:56 UTC Comment hidden (spam)