[PATCH #6407] Patch to resolve issue of non-booting Linux kernel

[PATCH #6407] Patch to resolve issue of non-booting Linux kernel

[Kris Van Hees]

Oracle has a full copyright assignment in place with the FSF.

This patch provides a fix for ticket #6407 'linux kernel linked with gold fails
to boot'.  The problem reported in the ticket was found to only occur when the
kernel is configured to be relocatable (CONFIG_RELOCATABLE=y).  Tracing through
the code, and comparison of kernels built with GNU ld vs GNU gold indicated
that there was a likely problem with the data generated when --emit-relocs is
passed to gold.  Further debugging showed that gold applies the value of
offset_in_output_section in the calculation of the new offset (it is
explicitly added to the new offset, but it is already accounted for in the
view address that is added to the new offset to determine the absolute address.

The patch below corrects this problem.  It was generated against cvs HEAD.

Earlier in the code, offset_in_output_section may be added to new_offset (if
not equal to -1).  If new_offset is to be an absolute address (executable or
shared object), view_address is added to new_offset, but because that already
includes the offset_in_output_section value, we need to subtract it again if
it had been added.

Testsuite execution has been verified to be identical between the unpatched and
the patched version.  No regressions were found.


	Cheers,
	Kris

ChangeLog entry:
================
2008-05-29  Kris Van Hees  <kris.van.hees@oracle.com>

        * target-reloc.h (relocate_for_relocatable): Fix new_offset calculation.

Patch:
======
--- binutils-head/gold/target-reloc.h-old	2008-05-29 16:20:54.000000000 -0400
+++ binutils-head/gold/target-reloc.h	2008-05-29 16:09:48.000000000 -0400
@@ -542,7 +542,11 @@
       // In an executable or dynamic object, generated by
       // --emit-relocs, r_offset is an absolute address.
       if (!parameters->options().relocatable())
-	new_offset += view_address;
+	{
+	  new_offset += view_address;
+	  if (offset_in_output_section != -1)
+	    new_offset -= offset_in_output_section;
+	}
 
       reloc_write.put_r_offset(new_offset);
       reloc_write.put_r_info(elfcpp::elf_r_info<size>(new_symndx, r_type));

[PATCH #6407] Patch to resolve issue of non-booting Linux kernel

[Kris Van Hees]

Oracle has a full copyright assignment in place with the FSF.

This patch provides a fix for ticket #6407 'linux kernel linked with gold fails
to boot'.  The problem reported in the ticket was found to only occur when the
kernel is configured to be relocatable (CONFIG_RELOCATABLE=y).  Tracing through
the code, and comparison of kernels built with GNU ld vs GNU gold indicated
that there was a likely problem with the data generated when --emit-relocs is
passed to gold.  Further debugging showed that gold applies the value of
offset_in_output_section in the calculation of the new offset (it is
explicitly added to the new offset, but it is already accounted for in the
view address that is added to the new offset to determine the absolute address.

The patch below corrects this problem.  It was generated against cvs HEAD.

Earlier in the code, offset_in_output_section may be added to new_offset (if
not equal to -1).  If new_offset is to be an absolute address (executable or
shared object), view_address is added to new_offset, but because that already
includes the offset_in_output_section value, we need to subtract it again if
it had been added.

Testsuite execution has been verified to be identical between the unpatched and
the patched version.  No regressions were found.


	Cheers,
	Kris

ChangeLog entry:
================
2008-05-29  Kris Van Hees  <kris.van.hees@oracle.com>

        * target-reloc.h (relocate_for_relocatable): Fix new_offset calculation.

Patch:
======
--- binutils-head/gold/target-reloc.h-old	2008-05-29 16:20:54.000000000 -0400
+++ binutils-head/gold/target-reloc.h	2008-05-29 16:09:48.000000000 -0400
@@ -542,7 +542,11 @@
       // In an executable or dynamic object, generated by
       // --emit-relocs, r_offset is an absolute address.
       if (!parameters->options().relocatable())
-	new_offset += view_address;
+	{
+	  new_offset += view_address;
+	  if (offset_in_output_section != -1)
+	    new_offset -= offset_in_output_section;
+	}
 
       reloc_write.put_r_offset(new_offset);
       reloc_write.put_r_info(elfcpp::elf_r_info<size>(new_symndx, r_type));

Re: [PATCH #6407] Patch to resolve issue of non-booting Linux kernel

[Ian Lance Taylor]

Kris Van Hees <kris.van.hees@oracle.com> writes:

> 2008-05-29  Kris Van Hees  <kris.van.hees@oracle.com>
>
>         * target-reloc.h (relocate_for_relocatable): Fix new_offset calculation.

Thanks a lot for looking into this.  This patch is OK.

The ChangeLog entry should refer to the PR number (and also the line
is slightly too long), as in:

2008-05-29  Kris Van Hees  <kris.van.hees@oracle.com>

	PR 6407
	* target-reloc.h (relocate_for_relocatable): Fix new_offset
	calculation.
	
Thanks!

Ian

Re: [PATCH #6407] Patch to resolve issue of non-booting Linux kernel

[Kris Van Hees]

On Thu, May 29, 2008 at 04:01:06PM -0700, Ian Lance Taylor wrote:
> Kris Van Hees <kris.van.hees@oracle.com> writes:
> 
> > 2008-05-29  Kris Van Hees  <kris.van.hees@oracle.com>
> >
> >         * target-reloc.h (relocate_for_relocatable): Fix new_offset calculation.
> 
> Thanks a lot for looking into this.  This patch is OK.

I currently do not have commit privs for binutils.  Do you want to check it
in on my behalf?  Since it is a pretty small patch I don't think it is right
to request commit privs based on it :)
> 
> The ChangeLog entry should refer to the PR number (and also the line
> is slightly too long), as in:
> 
> 2008-05-29  Kris Van Hees  <kris.van.hees@oracle.com>
> 
> 	PR 6407
> 	* target-reloc.h (relocate_for_relocatable): Fix new_offset
> 	calculation.

Good point.  Thanks.

	Thanks,
	Kris

Re: [PATCH #6407] Patch to resolve issue of non-booting Linux kernel

[Ian Lance Taylor]

Kris Van Hees <kris.van.hees@oracle.com> writes:

> On Thu, May 29, 2008 at 04:01:06PM -0700, Ian Lance Taylor wrote:
>> Kris Van Hees <kris.van.hees@oracle.com> writes:
>> 
>> > 2008-05-29  Kris Van Hees  <kris.van.hees@oracle.com>
>> >
>> >         * target-reloc.h (relocate_for_relocatable): Fix new_offset calculation.
>> 
>> Thanks a lot for looking into this.  This patch is OK.
>
> I currently do not have commit privs for binutils.  Do you want to check it
> in on my behalf?  Since it is a pretty small patch I don't think it is right
> to request commit privs based on it :)

I committed it.

Ian

[PATCH gold/6584]] Bad gzip magic number on boot of x86_64 kernel

[Kris Van Hees]

Oracle has a full copyright assignment in place with the FSF.

This patch provides a fix for ticket #6584 'kernel image decompression fails on
64bit machines (linked with gold)'.  Analysis of the problem showed that the
gzip'd image was found to be 8 bytes into the .data section, whereas a kernel
linked with the old ld placed the image 4 bytes into the .data section.
Further code analysis indicated that this discrepancy is due to gold applying
a 64-bit alignment to the image as it is encapsulated as an ELF object.  The
old linker uses an 8-bit alignment for the .data section of ELF encapsulated
binary data.

The patch below corrects this problem.  It was generated against cvs HEAD.
With this patch the kernel decompression completes successfully, and the kernel
execution commences.  My testing didn't result in a fully working system due
to other circumstances, but the testing did prove that the patch does resolve
the issue reported in this PR.

Testsuite execution has been verified to be identical between the unpatched and
the patched version.  No regressions were found.


	Cheers,
	Kris

ChangeLog entry:
================
2008-06-05  Kris Van Hees  <kris.van.hees@oracle.com>

	PR 6584
        * binary.cc (Binary_to_elf::sized_convert): Fix .data
	alignment.

Patch:
======
Index: gold/binary.cc
===================================================================
RCS file: /cvs/src/src/gold/binary.cc,v
retrieving revision 1.2
diff -u -r1.2 binary.cc
--- gold/binary.cc	2 Apr 2008 20:58:21 -0000	1.2
+++ gold/binary.cc	5 Jun 2008 15:18:41 -0000
@@ -207,7 +207,7 @@
 						| elfcpp::SHF_WRITE),
 					       data_offset,
 					       filesize, 0, 0,
-					       align, 0, &pout);
+					       1, 0, &pout);
   this->write_section_header<size, big_endian>(".symtab", &shstrtab,
 					       elfcpp::SHT_SYMTAB,
 					       0, symtab_offset, 4 * sym_size,

[PATCH gold/6584]] Bad gzip magic number on boot of x86_64 kernel

[Kris Van Hees]

Oracle has a full copyright assignment in place with the FSF.

This patch provides a fix for ticket #6584 'kernel image decompression fails on
64bit machines (linked with gold)'.  Analysis of the problem showed that the
gzip'd image was found to be 8 bytes into the .data section, whereas a kernel
linked with the old ld placed the image 4 bytes into the .data section.
Further code analysis indicated that this discrepancy is due to gold applying
a 64-bit alignment to the image as it is encapsulated as an ELF object.  The
old linker uses an 8-bit alignment for the .data section of ELF encapsulated
binary data.

The patch below corrects this problem.  It was generated against cvs HEAD.
With this patch the kernel decompression completes successfully, and the kernel
execution commences.  My testing didn't result in a fully working system due
to other circumstances, but the testing did prove that the patch does resolve
the issue reported in this PR.

Testsuite execution has been verified to be identical between the unpatched and
the patched version.  No regressions were found.


	Cheers,
	Kris

ChangeLog entry:
================
2008-06-05  Kris Van Hees  <kris.van.hees@oracle.com>

	PR 6584
        * binary.cc (Binary_to_elf::sized_convert): Fix .data
	alignment.

Patch:
======
Index: gold/binary.cc
===================================================================
RCS file: /cvs/src/src/gold/binary.cc,v
retrieving revision 1.2
diff -u -r1.2 binary.cc
--- gold/binary.cc	2 Apr 2008 20:58:21 -0000	1.2
+++ gold/binary.cc	5 Jun 2008 15:18:41 -0000
@@ -207,7 +207,7 @@
 						| elfcpp::SHF_WRITE),
 					       data_offset,
 					       filesize, 0, 0,
-					       align, 0, &pout);
+					       1, 0, &pout);
   this->write_section_header<size, big_endian>(".symtab", &shstrtab,
 					       elfcpp::SHT_SYMTAB,
 					       0, symtab_offset, 4 * sym_size,

Re: [PATCH gold/6584]] Bad gzip magic number on boot of x86_64 kernel

[Ian Lance Taylor]

Kris Van Hees <kris.van.hees@oracle.com> writes:

> 2008-06-05  Kris Van Hees  <kris.van.hees@oracle.com>
>
> 	PR 6584
>         * binary.cc (Binary_to_elf::sized_convert): Fix .data
> 	alignment.

Thanks!  Nice job.  Patch approved and applied.

Ian

[PATCH gold/6858] Patch to correctly handle undefined symbols in shlibs when linking -shared

[Kris Van Hees]

Oracle has a full copyright assignment in place with the FSF.

This patch provides the fix for PR 6858 'Linking with -shared should imply
-allow-shlib-undefined'.  The problem is a compatibility issue with the old
linker, which would enable allow-shlib-undefined behaviour if -shared is
passed unless the behaviour is explicitly disallowed

The patch below corrects this problem.  It was generated against cvs HEAD.

Testsuite execution has been verified to be identical between the unpatched and
the patched version.  No regressions were found.


	Cheers,
	Kris

ChangeLog entry:
================
2008-08-26  Kris Van Hees  <kris.van.hees@oracle.com>

	PR 6858
        * options.cc (General_options::finalize): Allow undefined symbols
	in shlibs if linking -shared.

Patch:
======
Index: gold/options.cc
===================================================================
RCS file: /cvs/src/src/gold/options.cc,v
retrieving revision 1.75
diff -r1.75 options.cc
766a767,769
>   if (this->shared() && !this->user_set_allow_shlib_undefined())
>     this->set_allow_shlib_undefined(true);
>

[PATCH gold/6859] Symbols specified as -u <symbol> may erroneously get added to dynsym

[Kris Van Hees]

Oracle has a full copyright assignment in place with the FSF.

This patch provides the fix for PR 6859 'Symbols specified as -u <symbol> may erroneously get added to the dynsym table'.  The problem turns out to be the
following:

Gold handles explicitly undefined symbols (-u <symbol>) differently from      
how the old GNU linker handles then.  With gold, if the symbol does not       
appear defined in any input object, it is added to both the symtab (symbol    
table) and the dynsym (dynamic symbol table) as an undefined symbol.  The     
old linker (correctly) only adds the symbol to the symtab in that case.  It   
works fine if the symbol gets defined in an input object.                     
                                                                             
The reason for this problem turns out to be the fact that during input object 
processing, "-u" symbols are checked as symbol tables are processed.  Then,   
all "-u" symbols are added as undefined symbols unless they were already      
seen (i.e. it adds all symbols that didn't appear in input object symbol      
tables).                                                                      
                                                                             
The fix for this problem turns out to be very easy: mark the "-u" symbols     
that are added after input object processing as not needing dynsym entries.

The patch below corrects this problem.  It was generated against cvs HEAD.

Testsuite execution has been verified to be identical between the unpatched and
the patched version.  No regressions were found.


	Cheers,
	Kris

ChangeLog entry:
================
2008-08-26  Kris Van Hees  <kris.van.hees@oracle.com>

	PR 6859
        * symtab.cc (Symbol::init_base_undefined): Mark explicitly undefined
	symbols as not needing a dynsym entry.

Patch:
======
Index: gold/symtab.cc
===================================================================
RCS file: /cvs/src/src/gold/symtab.cc,v
retrieving revision 1.107
diff -r1.107 symtab.cc
176a177
>   this->dynsym_index_ = -1;

Re: [PATCH gold/6858] Patch to correctly handle undefined symbols in shlibs when linking -shared

[Ian Lance Taylor]

Kris Van Hees <kris.van.hees@oracle.com> writes:

> 	PR 6858
>         * options.cc (General_options::finalize): Allow undefined symbols
> 	in shlibs if linking -shared.

This is OK.

Thanks.

Ian

Re: [PATCH gold/6859] Symbols specified as -u <symbol> may erroneously get added to dynsym

[Ian Lance Taylor]

Kris Van Hees <kris.van.hees@oracle.com> writes:

> 2008-08-26  Kris Van Hees  <kris.van.hees@oracle.com>
>
> 	PR 6859
>         * symtab.cc (Symbol::init_base_undefined): Mark explicitly undefined
> 	symbols as not needing a dynsym entry.
>
> Patch:
> ======
> Index: gold/symtab.cc
> ===================================================================
> RCS file: /cvs/src/src/gold/symtab.cc,v
> retrieving revision 1.107
> diff -r1.107 symtab.cc
> 176a177
>>   this->dynsym_index_ = -1;

Please write -1U.  This is OK with that change.

By the way, diffs are easier to understand if you generate them with
-up or plain -p.

Thanks.

Ian

Re: [PATCH gold/6859] Symbols specified as -u <symbol> may erroneously get added to dynsym

[Kris Van Hees]

[Resending this since my mail server seems to have eaten the previous copy.]

Revision of the patch.  Could you perform the commit, or could I ask for
sponsoring to get commit privileges and I'll commit it myself once they
are granted?  Same for the patch for PR 6858?

	Kris

Changelog:
----------
2008-08-26  Kris Van Hees  <kris.van.hees@oracle.com>

	PR 6859
	* symtab.cc (Symbol::init_base_undefined): Mark explicitly undefined
	symbols as not needing a dynsym entry.

Patch:
------
Index: gold/symtab.cc
===================================================================
RCS file: /cvs/src/src/gold/symtab.cc,v
retrieving revision 1.107
diff -u -p -r1.107 symtab.cc
--- gold/symtab.cc	23 Jul 2008 23:44:02 -0000	1.107
+++ gold/symtab.cc	26 Aug 2008 19:15:39 -0000
@@ -174,6 +174,7 @@ Symbol::init_base_undefined(const char* 
 			    elfcpp::STV visibility, unsigned char nonvis)
 {
   this->init_fields(name, version, type, binding, visibility, nonvis);
+  this->dynsym_index_ = -1U;
   this->source_ = IS_UNDEFINED;
   this->in_reg_ = true;
 }

On Tue, Aug 26, 2008 at 12:07:17PM -0700, Ian Lance Taylor wrote:
> Kris Van Hees <kris.van.hees@oracle.com> writes:
> 
> > Patch:
> > ======
> > Index: gold/symtab.cc
> > ===================================================================
> > RCS file: /cvs/src/src/gold/symtab.cc,v
> > retrieving revision 1.107
> > diff -r1.107 symtab.cc
> > 176a177
> >>   this->dynsym_index_ = -1;
> 
> Please write -1U.  This is OK with that change.
> 
> By the way, diffs are easier to understand if you generate them with
> -up or plain -p.
> 
> Thanks.
> 
> Ian