[PATCH] ld: fix .text section address alignment

[Roger Pau Monne]

According to the output from readelf, the .text section should be aligned to
16:

Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
  [ 0] (null)            NULL            00000000 000000 000000 00      0   0  0
  [ 1] .text             PROGBITS        000de300 000300 021d00 00  AX  0   0 16
[...]

This however doesn't seem to be enforced when the relocations are generated.
The following patch tries to address this by making sure the space used for
the relocations it also aligned to the same value as the .text section.

Signed-off-by: Roger Pau Monné 
Reported by: Ed Maste 
---
Cc: Kevin O'Connor 
---
 scripts/layoutrom.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/layoutrom.py b/scripts/layoutrom.py
index b976fb0..5534a9e 100755
--- a/scripts/layoutrom.py
+++ b/scripts/layoutrom.py
@@ -397,7 +397,7 @@ def writeLinkerScripts(li, out16, out32seg, out32flat):
                     + strRelocs("_reloc_rel", "code32init_start", relrelocs)
                     + strRelocs("_reloc_init", "code32flat_start", initrelocs))
         numrelocs = len(absrelocs + relrelocs + initrelocs)
-        sec32all_start -= numrelocs * 4
+        sec32all_start -= alignpos(numrelocs * 4, li.sec32low_align)
     filesections32flat = getSectionsFileid(li.sections, '32flat')
     out = outXRefs([], exportsyms=li.varlowsyms
                    , forcedelta=li.final_sec32low_start-li.sec32low_start)

[Kevin O'Connor]

On Tue, Feb 16, 2016 at 01:56:26PM +0100, Roger Pau Monne wrote:
> According to the output from readelf, the .text section should be aligned to
> 16:
>
> Section Headers:
>   [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
>   [ 0] (null)            NULL            00000000 000000 000000 00      0   0  0
>   [ 1] .text             PROGBITS        000de300 000300 021d00 00  AX  0   0 16
> [...]
>
> This however doesn't seem to be enforced when the relocations are generated.
> The following patch tries to address this by making sure the space used for
> the relocations it also aligned to the same value as the .text section.

Thanks.  What goes wrong if the .text section is not aligned?  The
code has already been assigned physical addresses by this point, so it
should not impact the runtime code.
> Signed-off-by: Roger Pau Monné 
> Reported by: Ed Maste
> Cc: Kevin O'Connor
>
> scripts/layoutrom.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/layoutrom.py b/scripts/layoutrom.py
> index b976fb0..5534a9e 100755
> --- a/scripts/layoutrom.py
> +++ b/scripts/layoutrom.py
> @@ -397,7 +397,7 @@ def writeLinkerScripts(li, out16, out32seg, out32flat):
>                      + strRelocs("_reloc_rel", "code32init_start", relrelocs)
>                      + strRelocs("_reloc_init", "code32flat_start", initrelocs))
>          numrelocs = len(absrelocs + relrelocs + initrelocs)
>    sec32all_start -= numrelocs * 4
>    sec32all_start -= alignpos(numrelocs * 4, li.sec32low_align)

I think this would need to find and use the maximum alignment of every
section that goes into ".text" and not just the alignment of
"sec32low" sections.

-Kevin

[Roger Pau Monné]

El 16/2/16 a les 17:33, Kevin O'Connor ha escrit:
> On Tue, Feb 16, 2016 at 01:56:26PM +0100, Roger Pau Monne wrote:
> > According to the output from readelf, the .text section should be aligned to
> > 16:
> >
> > Section Headers:
> >   [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
> >   [ 0] (null)            NULL            00000000 000000 000000 00      0   0  0
> >   [ 1] .text             PROGBITS        000de300 000300 021d00 00  AX  0   0 16
> > [...]
> >
> > This however doesn't seem to be enforced when the relocations are generated.
> > The following patch tries to address this by making sure the space used for
> > the relocations it also aligned to the same value as the .text section.
>
> Thanks.  What goes wrong if the .text section is not aligned?  The
> code has already been assigned physical addresses by this point, so it
> should not impact the runtime code.

It seems like ELF toolchain objcopy chokes if a section address is not
aligned to the alignment specified by the section, see:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207170

The snippet shown above has addr aligned to 16 (which matches latest
upstream), so it's not a problem, but the current SeaBIOS version
shipped in Xen 4.5 (1.7.5 IIRC) ends up with an addr that's not a
multiple of 16, as shown in the bug report, and objcopy complains with:

objcopy: elf_update() failed: Layout constraint violation

Roger.

[Kevin O'Connor]

On Tue, Feb 16, 2016 at 06:21:10PM +0100, Roger Pau Monné wrote:
> El 16/2/16 a les 17:33, Kevin O'Connor ha escrit:
> > On Tue, Feb 16, 2016 at 01:56:26PM +0100, Roger Pau Monne wrote:
> > > According to the output from readelf, the .text section should be aligned to
> > > 16:
> > >
> > > Section Headers:
> > >   [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
> > >   [ 0] (null)            NULL            00000000 000000 000000 00      0   0  0
> > >   [ 1] .text             PROGBITS        000de300 000300 021d00 00  AX  0   0 16
> > > [...]
> > >
> > > This however doesn't seem to be enforced when the relocations are generated.
> > > The following patch tries to address this by making sure the space used for
> > > the relocations it also aligned to the same value as the .text section.
> >
> > Thanks.  What goes wrong if the .text section is not aligned?  The
> > code has already been assigned physical addresses by this point, so it
> > should not impact the runtime code.
>
> It seems like ELF toolchain objcopy chokes if a section address is not
> aligned to the alignment specified by the section, see:
>
> https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207170
>
> The snippet shown above has addr aligned to 16 (which matches latest
> upstream), so it's not a problem, but the current SeaBIOS version
> shipped in Xen 4.5 (1.7.5 IIRC) ends up with an addr that's not a
> multiple of 16, as shown in the bug report, and objcopy complains with:
>
> objcopy: elf_update() failed: Layout constraint violation

Thanks.  I agree it should be fixed.  However, I think there are a few
other cases that could cause the ".text" section alignment to be off.
Are you okay with the patch below instead?

-Kevin

commit 3910de0dee216d5b5bf23cfa29bfc80d082b2ee7
Author: Kevin O'Connor 
Date:   Fri Feb 19 21:34:16 2016 -0500

build: fix .text section address alignment

Some linkers verify that sections have a start address that is aligned
    with the minimum alignment of that section.  Add extra padding to the
    ".text" section to ensure it is always aligned with the maximum
    alignment of any section placed in ".text".

Signed-off-by: Kevin O'Connor 
    Signed-off-by: Roger Pau Monné 
    Reported by: Ed Maste

diff --git a/scripts/layoutrom.py b/scripts/layoutrom.py
index b976fb0..6616721 100755
--- a/scripts/layoutrom.py
+++ b/scripts/layoutrom.py
@@ -34,18 +34,22 @@ COMMONTRAILER = """
 # Determine section locations
 ######################################################################

-# Align 'pos' to 'alignbytes' offset
+# Align 'pos' up to 'alignbytes' offset
 def alignpos(pos, alignbytes):
     mask = alignbytes - 1
     return (pos + mask) & ~mask

+# Align 'pos' down to 'alignbytes' offset
+def aligndown(pos, alignbytes):
+    mask = alignbytes - 1
+    return pos & ~mask
+
 # Determine the final addresses for a list of sections that end at an
 # address.
 def setSectionsStart(sections, endaddr, minalign=1, segoffset=0):
     totspace = 0
     for section in sections:
-        if section.align > minalign:
-            minalign = section.align
+        minalign = max(minalign, section.align)
         totspace = alignpos(totspace, section.align) + section.size
     startaddr = int((endaddr - totspace) / minalign) * minalign
     curaddr = startaddr
@@ -269,7 +273,7 @@ def doLayout(sections, config, genreloc):
         final_sec32low_end = BUILD_LOWRAM_END
         zonelow_base = final_sec32low_end - 64*1024
     relocdelta = final_sec32low_end - sec32low_end
-    li.sec32low_start, li.sec32low_align = setSectionsStart(
+    li.sec32low_start, sec32low_align = setSectionsStart(
         sections32low, sec32low_end, 16
         , segoffset=zonelow_base - relocdelta)
     li.sec32low_end = sec32low_end
@@ -405,6 +409,8 @@ def writeLinkerScripts(li, out16, out32seg, out32flat):
     if li.config.get('CONFIG_MULTIBOOT'):
         multiboot_header = "LONG(0x1BADB002) LONG(0) LONG(-0x1BADB002)"
         sec32all_start -= 3 * 4
+    sec32all_align = max([section.align for section in li.sections])
+    sec32all_start = aligndown(sec32all_start, sec32all_align)
     out += outXRefs(filesections32flat, exportsyms=[li.entrysym]) + """
     _reloc_min_align = 0x%x ;
     zonefseg_start = 0x%x ;

[Roger Pau Monné]

El 20/2/16 a les 3:41, Kevin O'Connor ha escrit:
> On Tue, Feb 16, 2016 at 06:21:10PM +0100, Roger Pau Monné wrote:
> > El 16/2/16 a les 17:33, Kevin O'Connor ha escrit:
> > > On Tue, Feb 16, 2016 at 01:56:26PM +0100, Roger Pau Monne wrote:
> > > > According to the output from readelf, the .text section should be aligned to
> > > > 16:
> > > >
> > > > Section Headers:
> > > >   [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
> > > >   [ 0] (null)            NULL            00000000 000000 000000 00      0   0  0
> > > >   [ 1] .text             PROGBITS        000de300 000300 021d00 00  AX  0   0 16
> > > > [...]
> > > >
> > > > This however doesn't seem to be enforced when the relocations are generated.
> > > > The following patch tries to address this by making sure the space used for
> > > > the relocations it also aligned to the same value as the .text section.
> > >
> > > Thanks.  What goes wrong if the .text section is not aligned?  The
> > > code has already been assigned physical addresses by this point, so it
> > > should not impact the runtime code.
> >
> > It seems like ELF toolchain objcopy chokes if a section address is not
> > aligned to the alignment specified by the section, see:
> >
> > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207170
> >
> > The snippet shown above has addr aligned to 16 (which matches latest
> > upstream), so it's not a problem, but the current SeaBIOS version
> > shipped in Xen 4.5 (1.7.5 IIRC) ends up with an addr that's not a
> > multiple of 16, as shown in the bug report, and objcopy complains with:
> >
> > objcopy: elf_update() failed: Layout constraint violation
>
> Thanks.  I agree it should be fixed.  However, I think there are a few
> other cases that could cause the ".text" section alignment to be off.
> Are you okay with the patch below instead?

Yes, looks fine to me. AFAICT SeaBIOS packs all the sections (.text,
.data, .rodata) ibnside of the .text section, which I didn't realize before.

Roger

[Kevin O'Connor]

On Mon, Feb 22, 2016 at 12:07:00PM +0100, Roger Pau Monné wrote:
> El 20/2/16 a les 3:41, Kevin O'Connor ha escrit:
> > On Tue, Feb 16, 2016 at 06:21:10PM +0100, Roger Pau Monné wrote:
> > > El 16/2/16 a les 17:33, Kevin O'Connor ha escrit:
> > > > On Tue, Feb 16, 2016 at 01:56:26PM +0100, Roger Pau Monne wrote:
> > > > > According to the output from readelf, the .text section should be aligned to
> > > > > 16:
> > > > >
> > > > > Section Headers:
> > > > >   [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
> > > > >   [ 0] (null)            NULL            00000000 000000 000000 00      0   0  0
> > > > >   [ 1] .text             PROGBITS        000de300 000300 021d00 00  AX  0   0 16
> > > > > [...]
> > > > >
> > > > > This however doesn't seem to be enforced when the relocations are generated.
> > > > > The following patch tries to address this by making sure the space used for
> > > > > the relocations it also aligned to the same value as the .text section.
> > > >
> > > > Thanks.  What goes wrong if the .text section is not aligned?  The
> > > > code has already been assigned physical addresses by this point, so it
> > > > should not impact the runtime code.
> > >
> > > It seems like ELF toolchain objcopy chokes if a section address is not
> > > aligned to the alignment specified by the section, see:
> > >
> > > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207170
> > >
> > > The snippet shown above has addr aligned to 16 (which matches latest
> > > upstream), so it's not a problem, but the current SeaBIOS version
> > > shipped in Xen 4.5 (1.7.5 IIRC) ends up with an addr that's not a
> > > multiple of 16, as shown in the bug report, and objcopy complains with:
> > >
> > > objcopy: elf_update() failed: Layout constraint violation
> >
> > Thanks.  I agree it should be fixed.  However, I think there are a few
> > other cases that could cause the ".text" section alignment to be off.
> > Are you okay with the patch below instead?
>
> Yes, looks fine to me. AFAICT SeaBIOS packs all the sections (.text,
> .data, .rodata) ibnside of the .text section, which I didn't realize before.

Thanks, I committed the change.

-Kevin

[Roger Pau Monné]

El 23/2/16 a les 15:53, Kevin O'Connor ha escrit:
> On Mon, Feb 22, 2016 at 12:07:00PM +0100, Roger Pau Monné wrote:
> > El 20/2/16 a les 3:41, Kevin O'Connor ha escrit:
> > > On Tue, Feb 16, 2016 at 06:21:10PM +0100, Roger Pau Monné wrote:
> > > > El 16/2/16 a les 17:33, Kevin O'Connor ha escrit:
> > > > > On Tue, Feb 16, 2016 at 01:56:26PM +0100, Roger Pau Monne wrote:
> > > > > > According to the output from readelf, the .text section should be aligned to
> > > > > > 16:
> > > > > >
> > > > > > Section Headers:
> > > > > >   [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
> > > > > >   [ 0] (null)            NULL            00000000 000000 000000 00      0   0  0
> > > > > >   [ 1] .text             PROGBITS        000de300 000300 021d00 00  AX  0   0 16
> > > > > > [...]
> > > > > >
> > > > > > This however doesn't seem to be enforced when the relocations are generated.
> > > > > > The following patch tries to address this by making sure the space used for
> > > > > > the relocations it also aligned to the same value as the .text section.
> > > > >
> > > > > Thanks.  What goes wrong if the .text section is not aligned?  The
> > > > > code has already been assigned physical addresses by this point, so it
> > > > > should not impact the runtime code.
> > > >
> > > > It seems like ELF toolchain objcopy chokes if a section address is not
> > > > aligned to the alignment specified by the section, see:
> > > >
> > > > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207170
> > > >
> > > > The snippet shown above has addr aligned to 16 (which matches latest
> > > > upstream), so it's not a problem, but the current SeaBIOS version
> > > > shipped in Xen 4.5 (1.7.5 IIRC) ends up with an addr that's not a
> > > > multiple of 16, as shown in the bug report, and objcopy complains with:
> > > >
> > > > objcopy: elf_update() failed: Layout constraint violation
> > >
> > > Thanks.  I agree it should be fixed.  However, I think there are a few
> > > other cases that could cause the ".text" section alignment to be off.
> > > Are you okay with the patch below instead?
> >
> > Yes, looks fine to me. AFAICT SeaBIOS packs all the sections (.text,
> > .data, .rodata) ibnside of the .text section, which I didn't realize before.
>
> Thanks, I committed the change.

Thanks, I would also like to request this fix to be backported to stable
branches. Should I send a formal request, or is this email enough?

Ideally I would like to see it applied to 1.9, 1.8 and 1.7.5.

Roger.

[Kevin O'Connor]

On Tue, Feb 23, 2016 at 04:06:20PM +0100, Roger Pau Monné wrote:
> El 23/2/16 a les 15:53, Kevin O'Connor ha escrit:
> > On Mon, Feb 22, 2016 at 12:07:00PM +0100, Roger Pau Monné wrote:
> > > El 20/2/16 a les 3:41, Kevin O'Connor ha escrit:
> > > > On Tue, Feb 16, 2016 at 06:21:10PM +0100, Roger Pau Monné wrote:
> > > > > It seems like ELF toolchain objcopy chokes if a section address is not
> > > > > aligned to the alignment specified by the section, see:
> > > > >
> > > > > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207170
> > > > >
> > > > > The snippet shown above has addr aligned to 16 (which matches latest
> > > > > upstream), so it's not a problem, but the current SeaBIOS version
> > > > > shipped in Xen 4.5 (1.7.5 IIRC) ends up with an addr that's not a
> > > > > multiple of 16, as shown in the bug report, and objcopy complains with:
> > > > >
> > > > > objcopy: elf_update() failed: Layout constraint violation
> > > >
> > > > Thanks.  I agree it should be fixed.  However, I think there are a few
> > > > other cases that could cause the ".text" section alignment to be off.
> > > > Are you okay with the patch below instead?
> > >
> > > Yes, looks fine to me. AFAICT SeaBIOS packs all the sections (.text,
> > > .data, .rodata) ibnside of the .text section, which I didn't realize before.
> >
> > Thanks, I committed the change.
>
> Thanks, I would also like to request this fix to be backported to stable
> branches. Should I send a formal request, or is this email enough?
>
> Ideally I would like to see it applied to 1.9, 1.8 and 1.7.5.

Gerd maintains the stable trees.

I don't know Gerd's thoughts on supporting older stable branches, but
I can say that the build changed significantly after 1.7.5 and it
would require more than a simple backport of this patch.

-Kevin

[Gerd Hoffmann]

Hi,
> > > Thanks, I committed the change.
> >
> > Thanks, I would also like to request this fix to be backported to stable
> > branches. Should I send a formal request, or is this email enough?
> >
> > Ideally I would like to see it applied to 1.9, 1.8 and 1.7.5.
>
> Gerd maintains the stable trees.

cherry-picked into 1.9-stable branch.
> I don't know Gerd's thoughts on supporting older stable branches, but
> I can say that the build changed significantly after 1.7.5 and it
> would require more than a simple backport of this patch.

It's just one stable branch atm, and I can't see a good reason to
maintain multiple stable branches.  seabios rarely has disruptive
changes and is supposed to be backward compatible, so just updating to
1.9.x should work (if it doesn't it is most likely a bug).

cheers,
  Gerd