PATCH: Fix gnu-efi-3.0b-041222 for ia32

PATCH: Fix gnu-efi-3.0b-041222 for ia32

[H. J. Lu]

I don't know where I should report gnu-efi bug. The ia32 support in
gnu-efi-3.0b-041222 doesn't work with newer gcc and binutils. The
problem is in the linker script. The new gcc may put functions in
.text* sections. The newer linker handles orphaned sections
differently. I tested this patch on both ia32 and ia64 with different
versions of gcc and binutils by building elilo 3.6. Elilo 3.6 loads
and run kernel correctly on ia64. It seems to work find on ia32.

Thanks.


H.J.
--- gnu-efi/gnuefi/elf_ia32_efi.lds.ia32	2004-12-22 07:52:28.000000000 -0800
+++ gnu-efi/gnuefi/elf_ia32_efi.lds	2006-03-17 11:47:21.000000000 -0800
@@ -9,7 +9,11 @@ SECTIONS
   . = ALIGN(4096);
   .text :
   {
-   *(.text)
+   *(.text*)
+  }
+  .reloc :
+  {
+   *(.reloc)
   }
   . = ALIGN(4096);
   .data :
@@ -48,12 +52,9 @@ SECTIONS
   . = ALIGN(4096);
   .dynstr   : { *(.dynstr) }
   . = ALIGN(4096);
-  .ignored.reloc :
+  /DISCARD/ :
   {
     *(.rel.reloc)
-  }
-  .ignored :
-  {
     *(.eh_frame)
   }
 }
--- gnu-efi/gnuefi/elf_ia64_efi.lds.ia32	2004-12-22 10:18:10.000000000 -0800
+++ gnu-efi/gnuefi/elf_ia64_efi.lds	2006-03-17 11:51:51.000000000 -0800
@@ -59,13 +59,10 @@ SECTIONS
   .dynsym   : { *(.dynsym) }
   . = ALIGN(4096);
   .dynstr   : { *(.dynstr) }
-  .ignored_reloc :
+  /DISCARD/ :
   {
     *(.rela.plabel)
     *(.rela.reloc)
-  }
-  /DISCARD/ :
-  {
     *(.IA_64.unwind*)
     *(.IA64.unwind*)
   }

Re: PATCH: Fix gnu-efi-3.0b-041222 for ia32

[James E Wilson]

On Fri, 2006-03-17 at 12:23, H. J. Lu wrote:
> -   *(.text)
> +   *(.text*)

That should be .text.*, not .text*.  The distinction here is important.
Admittedly, it will probably never matter for gnu-efi, but get this
wrong in binutils and you will break some end user code.

Re: PATCH: Fix gnu-efi-3.0b-041222 for ia32

[James E Wilson]

On Fri, 2006-03-17 at 12:45, James E Wilson wrote:
> That should be .text.*, not .text*.

And because .text.* doesn't match .text, this actually needs to be
    *(.text .text.*)
Try looking at ld --verbose output, and grepping for .text.
-- 
Jim Wilson, GNU Tools Support, http://www.specifix.com

Re: PATCH: Fix gnu-efi-3.0b-041222 for ia32

[H. J. Lu]

On Fri, Mar 17, 2006 at 12:53:24PM -0800, James E Wilson wrote:
> On Fri, 2006-03-17 at 12:45, James E Wilson wrote:
> > That should be .text.*, not .text*.
> 
> And because .text.* doesn't match .text, this actually needs to be
>     *(.text .text.*)

I don't think so. linker calls fnmatch to match section names by
patterns.  According to my man page:

NAME
       fnmatch - match filename or pathname

SYNOPSIS
       #include <fnmatch.h>

       int fnmatch(const char *pattern, const char *string, int flags);

DESCRIPTION
       The  fnmatch()  function checks whether the string argument
matches the
       pattern argument, which is a shell wildcard pattern.

".text*" should work as a shell wildcard pattern.

> Try looking at ld --verbose output, and grepping for .text.

This is the updated patch to include *(.gnu.linkonce.t.*) sections.



H.J.
---
--- gnu-efi/gnuefi/elf_ia32_efi.lds.ia32	2004-12-22 07:52:28.000000000 -0800
+++ gnu-efi/gnuefi/elf_ia32_efi.lds	2006-03-17 13:30:58.000000000 -0800
@@ -9,7 +9,12 @@ SECTIONS
   . = ALIGN(4096);
   .text :
   {
-   *(.text)
+   *(.text*)
+   *(.gnu.linkonce.t.*)
+  }
+  .reloc :
+  {
+   *(.reloc)
   }
   . = ALIGN(4096);
   .data :
@@ -48,12 +53,9 @@ SECTIONS
   . = ALIGN(4096);
   .dynstr   : { *(.dynstr) }
   . = ALIGN(4096);
-  .ignored.reloc :
+  /DISCARD/ :
   {
     *(.rel.reloc)
-  }
-  .ignored :
-  {
     *(.eh_frame)
   }
 }
--- gnu-efi/gnuefi/elf_ia64_efi.lds.ia32	2004-12-22 10:18:10.000000000 -0800
+++ gnu-efi/gnuefi/elf_ia64_efi.lds	2006-03-17 13:31:11.000000000 -0800
@@ -9,7 +9,7 @@ SECTIONS
   . = ALIGN(4096);
   .text :
   {
-   *(.text)
+   *(.text*)
    *(.gnu.linkonce.t*)
   }
   . = ALIGN(4096);
@@ -59,13 +59,10 @@ SECTIONS
   .dynsym   : { *(.dynsym) }
   . = ALIGN(4096);
   .dynstr   : { *(.dynstr) }
-  .ignored_reloc :
+  /DISCARD/ :
   {
     *(.rela.plabel)
     *(.rela.reloc)
-  }
-  /DISCARD/ :
-  {
     *(.IA_64.unwind*)
     *(.IA64.unwind*)
   }

Re: PATCH: Fix gnu-efi-3.0b-041222 for ia32

[H. J. Lu]

On Fri, Mar 17, 2006 at 01:46:57PM -0800, H. J. Lu wrote:
> On Fri, Mar 17, 2006 at 12:53:24PM -0800, James E Wilson wrote:
> > On Fri, 2006-03-17 at 12:45, James E Wilson wrote:
> > > That should be .text.*, not .text*.
> > 
> > And because .text.* doesn't match .text, this actually needs to be
> >     *(.text .text.*)
> 
> I don't think so. linker calls fnmatch to match section names by
> patterns.  According to my man page:
> 
> NAME
>        fnmatch - match filename or pathname
> 
> SYNOPSIS
>        #include <fnmatch.h>
> 
>        int fnmatch(const char *pattern, const char *string, int flags);
> 
> DESCRIPTION
>        The  fnmatch()  function checks whether the string argument
> matches the
>        pattern argument, which is a shell wildcard pattern.
> 
> ".text*" should work as a shell wildcard pattern.
> 
> > Try looking at ld --verbose output, and grepping for .text.
> 
> This is the updated patch to include *(.gnu.linkonce.t.*) sections.
> 

This new makes linker scripts similar to normal linker. It should
generate the same output.


H.J.
---
--- gnu-efi/gnuefi/elf_ia32_efi.lds.ia32	2004-12-22 07:52:28.000000000 -0800
+++ gnu-efi/gnuefi/elf_ia32_efi.lds	2006-03-17 13:48:13.000000000 -0800
@@ -10,6 +10,12 @@ SECTIONS
   .text :
   {
    *(.text)
+   *(.text.*)
+   *(.gnu.linkonce.t.*)
+  }
+  .reloc :
+  {
+   *(.reloc)
   }
   . = ALIGN(4096);
   .data :
@@ -48,12 +54,9 @@ SECTIONS
   . = ALIGN(4096);
   .dynstr   : { *(.dynstr) }
   . = ALIGN(4096);
-  .ignored.reloc :
+  /DISCARD/ :
   {
     *(.rel.reloc)
-  }
-  .ignored :
-  {
     *(.eh_frame)
   }
 }
--- gnu-efi/gnuefi/elf_ia64_efi.lds.ia32	2004-12-22 10:18:10.000000000 -0800
+++ gnu-efi/gnuefi/elf_ia64_efi.lds	2006-03-17 13:48:20.000000000 -0800
@@ -10,6 +10,7 @@ SECTIONS
   .text :
   {
    *(.text)
+   *(.text.*)
    *(.gnu.linkonce.t*)
   }
   . = ALIGN(4096);
@@ -59,13 +60,10 @@ SECTIONS
   .dynsym   : { *(.dynsym) }
   . = ALIGN(4096);
   .dynstr   : { *(.dynstr) }
-  .ignored_reloc :
+  /DISCARD/ :
   {
     *(.rela.plabel)
     *(.rela.reloc)
-  }
-  /DISCARD/ :
-  {
     *(.IA_64.unwind*)
     *(.IA64.unwind*)
   }

Re: PATCH: Fix gnu-efi-3.0b-041222 for ia32

[Matthew Wilcox]

On Fri, Mar 17, 2006 at 01:46:57PM -0800, H. J. Lu wrote:
> On Fri, Mar 17, 2006 at 12:53:24PM -0800, James E Wilson wrote:
> > On Fri, 2006-03-17 at 12:45, James E Wilson wrote:
> > > That should be .text.*, not .text*.
> > 
> > And because .text.* doesn't match .text, this actually needs to be
> >     *(.text .text.*)
> 
> I don't think so. linker calls fnmatch to match section names by
> patterns.  According to my man page:
> 
> DESCRIPTION
>        The  fnmatch()  function checks whether the string argument
> matches the
>        pattern argument, which is a shell wildcard pattern.
> 
> ".text*" should work as a shell wildcard pattern.

I believe his point is that .text* matches .textual and .texterific and
many other sections which shouldn't be matched.  So you need to match
.text and .text.*, not just .text*

Re: PATCH: Fix gnu-efi-3.0b-041222 for ia32

[James E Wilson]

On Fri, 2006-03-17 at 13:52, Matthew Wilcox wrote:
> I believe his point is that .text* matches .textual and .texterific and
> many other sections which shouldn't be matched.  So you need to match
> .text and .text.*, not just .text*

Exactly.

The best example I can give is that I've fixed broken linker scripts in
libgloss that tried to match linkonce small data sections using
.gnu.linkonce.s*, without realizing that linkonce small bss sections are
.gnu.linkonce.sb.*.  This caused some C++ programs to fail in strange
and difficult to debug ways.  Those dots are important.
-- 
Jim Wilson, GNU Tools Support, http://www.specifix.com

Re: PATCH: Fix gnu-efi-3.0b-041222 for ia32

[James E Wilson]

On Fri, 2006-03-17 at 13:52, H. J. Lu wrote:
>     *(.gnu.linkonce.t*)

This isn't something you added, but should be fixed just the same.  This
will accidentally cause some thread-local data sections to be merged
into the text section.  The thread-local sections may not have existed
when this code was written, but they do now.  .gnu.linkonce.td.* is used
for linkonce thread local data, and .gnu.linkonce.tb.* is used for
linkonce thread local bss.
-- 
Jim Wilson, GNU Tools Support, http://www.specifix.com

Re: PATCH: Fix gnu-efi-3.0b-041222 for ia32

[Stephane Eranian]

H.J.,

On Fri, Mar 17, 2006 at 12:23:01PM -0800, H. J. Lu wrote:
> I don't know where I should report gnu-efi bug. The ia32 support in
> gnu-efi-3.0b-041222 doesn't work with newer gcc and binutils. The
> problem is in the linker script. The new gcc may put functions in
> .text* sections. The newer linker handles orphaned sections
> differently. I tested this patch on both ia32 and ia64 with different
> versions of gcc and binutils by building elilo 3.6. Elilo 3.6 loads
> and run kernel correctly on ia64. It seems to work find on ia32.
> 
As far as I know, I am still the maintainer of this package. But 
I need to check with some of our Linux people at HP.

It is unfortunate that this package as such strong dependency
of the GNU compiler and linker. This has caused a lot of troubles
when all the recent updates to gcc. Yet I do not see a way around
this.

Once you and James ettle on the right update I'll put it in
and push a new package.

Thanks for tracking this down.

> 
> H.J.
> --- gnu-efi/gnuefi/elf_ia32_efi.lds.ia32	2004-12-22 07:52:28.000000000 -0800
> +++ gnu-efi/gnuefi/elf_ia32_efi.lds	2006-03-17 11:47:21.000000000 -0800
> @@ -9,7 +9,11 @@ SECTIONS
>    . = ALIGN(4096);
>    .text :
>    {
> -   *(.text)
> +   *(.text*)
> +  }
> +  .reloc :
> +  {
> +   *(.reloc)
>    }
>    . = ALIGN(4096);
>    .data :
> @@ -48,12 +52,9 @@ SECTIONS
>    . = ALIGN(4096);
>    .dynstr   : { *(.dynstr) }
>    . = ALIGN(4096);
> -  .ignored.reloc :
> +  /DISCARD/ :
>    {
>      *(.rel.reloc)
> -  }
> -  .ignored :
> -  {
>      *(.eh_frame)
>    }
>  }
> --- gnu-efi/gnuefi/elf_ia64_efi.lds.ia32	2004-12-22 10:18:10.000000000 -0800
> +++ gnu-efi/gnuefi/elf_ia64_efi.lds	2006-03-17 11:51:51.000000000 -0800
> @@ -59,13 +59,10 @@ SECTIONS
>    .dynsym   : { *(.dynsym) }
>    . = ALIGN(4096);
>    .dynstr   : { *(.dynstr) }
> -  .ignored_reloc :
> +  /DISCARD/ :
>    {
>      *(.rela.plabel)
>      *(.rela.reloc)
> -  }
> -  /DISCARD/ :
> -  {
>      *(.IA_64.unwind*)
>      *(.IA64.unwind*)
>    }

-- 

-Stephane

Re: PATCH: Fix gnu-efi-3.0b-041222 for ia32

[H. J. Lu]

On Fri, Mar 17, 2006 at 03:15:48PM -0800, James E Wilson wrote:
> On Fri, 2006-03-17 at 13:52, H. J. Lu wrote:
> >     *(.gnu.linkonce.t*)
> 
> This isn't something you added, but should be fixed just the same.  This
> will accidentally cause some thread-local data sections to be merged
> into the text section.  The thread-local sections may not have existed
> when this code was written, but they do now.  .gnu.linkonce.td.* is used
> for linkonce thread local data, and .gnu.linkonce.tb.* is used for
> linkonce thread local bss.

I guess we never used TLS in EFI applications before. It doesn't
really matter since TLS needs more than the linker script change.
Here is the new patch.


H.J.
---
--- gnu-efi/gnuefi/elf_ia32_efi.lds.ia32	2004-12-22 07:52:28.000000000 -0800
+++ gnu-efi/gnuefi/elf_ia32_efi.lds	2006-03-17 13:48:13.000000000 -0800
@@ -10,6 +10,12 @@ SECTIONS
   .text :
   {
    *(.text)
+   *(.text.*)
+   *(.gnu.linkonce.t.*)
+  }
+  .reloc :
+  {
+   *(.reloc)
   }
   . = ALIGN(4096);
   .data :
@@ -48,12 +54,9 @@ SECTIONS
   . = ALIGN(4096);
   .dynstr   : { *(.dynstr) }
   . = ALIGN(4096);
-  .ignored.reloc :
+  /DISCARD/ :
   {
     *(.rel.reloc)
-  }
-  .ignored :
-  {
     *(.eh_frame)
   }
 }
--- gnu-efi/gnuefi/elf_ia64_efi.lds.ia32	2004-12-22 10:18:10.000000000 -0800
+++ gnu-efi/gnuefi/elf_ia64_efi.lds	2006-03-17 15:27:48.000000000 -0800
@@ -10,7 +10,8 @@ SECTIONS
   .text :
   {
    *(.text)
-   *(.gnu.linkonce.t*)
+   *(.text.*)
+   *(.gnu.linkonce.t.*)
   }
   . = ALIGN(4096);
   __gp = ALIGN (8) + 0x200000;
@@ -59,13 +60,10 @@ SECTIONS
   .dynsym   : { *(.dynsym) }
   . = ALIGN(4096);
   .dynstr   : { *(.dynstr) }
-  .ignored_reloc :
+  /DISCARD/ :
   {
     *(.rela.plabel)
     *(.rela.reloc)
-  }
-  /DISCARD/ :
-  {
     *(.IA_64.unwind*)
     *(.IA64.unwind*)
   }

Re: PATCH: Fix gnu-efi-3.0b-041222 for ia32

[James E Wilson]

On Fri, 2006-03-17 at 15:31, H. J. Lu wrote:
> I guess we never used TLS in EFI applications before. It doesn't
> really matter since TLS needs more than the linker script change.

True, but there is always the danger that someone might copy this linker
script somewhere else where it does matter.  So we may as well get it
right.

This version looks good to me.
-- 
Jim Wilson, GNU Tools Support, http://www.specifix.com