public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] (version 2) Change MIPS linker stubs to allow for more than  2^15 symbols.
@ 2006-06-08 23:03 David Daney
  2006-06-08 23:12 ` Thiemo Seufer
                   ` (2 more replies)
  0 siblings, 3 replies; 28+ messages in thread
From: David Daney @ 2006-06-08 23:03 UTC (permalink / raw)
  To: binutils; +Cc: Richard Sandiford, Thiemo Seufer, Daniel Jacobowitz

[-- Attachment #1: Type: text/plain, Size: 2022 bytes --]

As noted in this thread:

http://sourceware.org/ml/binutils/2006-06/msg00086.html

The mips linker would not reliably allow for more than 2^15 dynamic 
symbols.  This prevents GCC's libgcj java runtime library from being 
correctly linked.

This new version of the patch fixes allows up to 2^31 - 1 dynamic 
symbols by modifying linker stub generation.

If there are more than 2^16 dynamic symbols (as is the case for 
libgcj.so) a 20 byte stub is used.

For objects with 2^16 or fewer dynamic symbols, a stub of the original 
16 byte size is used.  This case is further divided in to two sub-cases: 
For an index value less than 2^15 the original sign extending stub is 
used, for index values of 2^15 through 2^16 a new non sign extending 
(but still 16 byte) stub is used.  The idea is to make the stub as small 
as possible as well as not change its form unless necessary.  It is 
thought that this strategy will decrease the chance of breaking existing 
tools.

Tested mipsel-linux cross build running on i686-pc-linux-gnu with make 
-k check showing no regressions.  Also tested that java executables from 
GCC-4.2 now are able to run.  Small objects still have the original 16 
byte stub, libgcj.so.7.0.0 uses the 20 byte stub.

Perhaps others could test on mips64 and/or big-endian systems.

OK to commit?

I know it is a bit late in the game, but how about for 2.17?  It would 
be nice to say that gcc-4.2 can be used with an official binutils release.

David Daney

2006-06-08  David Daney  <ddaney@avtrex.com>

	* elfxx-mips.c (STUB_LI16): Removed.
	(STUB_LUI): New macro.
	(STUB_LI16U): Ditto.
	(STUB_LI16S): Ditto.
	(MIPS_FUNCTION_STUB_SIZE): Rewrote to take info parameter.
	(_bfd_mips_elf_adjust_dynamic_symbol): Pass info parameter to
	MIPS_FUNCTION_STUB_SIZE.
	(_bfd_mips_elf_always_size_sections): Ditto.
	(_bfd_mips_elf_size_dynamic_sections): Ditto.
	(_bfd_mips_elf_finish_dynamic_sections): Ditto.
	(_bfd_mips_elf_finish_dynamic_symbol): Rewrote stub generation
	to allow larger symbol table indexes.

[-- Attachment #2: mips-stubs.diff.txt --]
[-- Type: text/plain, Size: 7963 bytes --]

Index: elfxx-mips.c
===================================================================
RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
retrieving revision 1.169
diff -c -p -r1.169 elfxx-mips.c
*** elfxx-mips.c	22 May 2006 15:06:22 -0000	1.169
--- elfxx-mips.c	8 Jun 2006 21:50:00 -0000
*************** static bfd *reldyn_sorting_bfd;
*** 623,642 ****
  #define MIPS_ELF_GOT_MAX_SIZE(INFO) (ELF_MIPS_GP_OFFSET (INFO) + 0x7fff)
  
  /* Instructions which appear in a stub.  */
! #define STUB_LW(abfd)						\
!   ((ABI_64_P (abfd)  						\
!     ? 0xdf998010		/* ld t9,0x8010(gp) */		\
!     : 0x8f998010))              /* lw t9,0x8010(gp) */
! #define STUB_MOVE(abfd)                                         \
!    ((ABI_64_P (abfd)						\
!      ? 0x03e0782d		/* daddu t7,ra */		\
!      : 0x03e07821))		/* addu t7,ra */
! #define STUB_JALR 0x0320f809	/* jalr t9,ra */
! #define STUB_LI16(abfd)                                         \
!   ((ABI_64_P (abfd)						\
!    ? 0x64180000			/* daddiu t8,zero,0 */		\
!    : 0x24180000))		/* addiu t8,zero,0 */
! #define MIPS_FUNCTION_STUB_SIZE (16)
  
  /* The name of the dynamic interpreter.  This is put in the .interp
     section.  */
--- 623,648 ----
  #define MIPS_ELF_GOT_MAX_SIZE(INFO) (ELF_MIPS_GP_OFFSET (INFO) + 0x7fff)
  
  /* Instructions which appear in a stub.  */
! #define STUB_LW(abfd)							\
!   ((ABI_64_P (abfd)							\
!     ? 0xdf998010				/* ld t9,0x8010(gp) */	\
!     : 0x8f998010))              		/* lw t9,0x8010(gp) */
! #define STUB_MOVE(abfd)							\
!    ((ABI_64_P (abfd)							\
!      ? 0x03e0782d				/* daddu t7,ra */	\
!      : 0x03e07821))				/* addu t7,ra */
! #define STUB_LUI(VAL) (0x3c180000 + (VAL))	/* lui t8,VAL */
! #define STUB_JALR 0x0320f809			/* jalr t9,ra */
! #define STUB_LI16U(VAL) (0x34180000 + (VAL))	/* ori t8,zero,VAL unsigned*/
! #define STUB_LI16S(abfd, VAL)						\
!    ((ABI_64_P (abfd)							\
!     ? (0x64180000 + (VAL))	/* daddiu t8,zero,VAL sign extended */	\
!     : (0x24180000 + (VAL))))	/* addiu t8,zero,VAL sign extended */
! 
! #define MIPS_FUNCTION_STUB_SIZE(INFO) \
!   (elf_hash_table (INFO)->dynsymcount > 65536 ? 20 : 16)
! 
! #define MIPS_FUNCTION_STUB_MAX_SIZE 20
  
  /* The name of the dynamic interpreter.  This is put in the .interp
     section.  */
*************** _bfd_mips_elf_adjust_dynamic_symbol (str
*** 6877,6883 ****
  	  h->plt.offset = s->size;
  
  	  /* Make room for this stub code.  */
! 	  s->size += MIPS_FUNCTION_STUB_SIZE;
  
  	  /* The last half word of the stub will be filled with the index
  	     of this symbol in .dynsym section.  */
--- 6883,6889 ----
  	  h->plt.offset = s->size;
  
  	  /* Make room for this stub code.  */
! 	  s->size += MIPS_FUNCTION_STUB_SIZE (info);
  
  	  /* The last half word of the stub will be filled with the index
  	     of this symbol in .dynsym section.  */
*************** _bfd_mips_elf_always_size_sections (bfd 
*** 7142,7148 ****
    /* In the worst case, we'll get one stub per dynamic symbol, plus
       one to account for the dummy entry at the end required by IRIX
       rld.  */
!   loadable_size += MIPS_FUNCTION_STUB_SIZE * (i + 1);
  
    if (htab->is_vxworks)
      /* There's no need to allocate page entries for VxWorks; R_MIPS_GOT16
--- 7148,7154 ----
    /* In the worst case, we'll get one stub per dynamic symbol, plus
       one to account for the dummy entry at the end required by IRIX
       rld.  */
!   loadable_size += MIPS_FUNCTION_STUB_SIZE (info) * (i + 1);
  
    if (htab->is_vxworks)
      /* There's no need to allocate page entries for VxWorks; R_MIPS_GOT16
*************** _bfd_mips_elf_size_dynamic_sections (bfd
*** 7360,7366 ****
  	{
  	  /* IRIX rld assumes that the function stub isn't at the end
  	     of .text section. So put a dummy. XXX  */
! 	  s->size += MIPS_FUNCTION_STUB_SIZE;
  	}
        else if (! info->shared
  	       && ! mips_elf_hash_table (info)->use_rld_obj_head
--- 7366,7372 ----
  	{
  	  /* IRIX rld assumes that the function stub isn't at the end
  	     of .text section. So put a dummy. XXX  */
! 	  s->size += MIPS_FUNCTION_STUB_SIZE (info);
  	}
        else if (! info->shared
  	       && ! mips_elf_hash_table (info)->use_rld_obj_head
*************** _bfd_mips_elf_finish_dynamic_symbol (bfd
*** 7997,8009 ****
    asection *sgot;
    struct mips_got_info *g, *gg;
    const char *name;
  
    dynobj = elf_hash_table (info)->dynobj;
  
    if (h->plt.offset != MINUS_ONE)
      {
        asection *s;
!       bfd_byte stub[MIPS_FUNCTION_STUB_SIZE];
  
        /* This symbol has a stub.  Set it up.  */
  
--- 8003,8016 ----
    asection *sgot;
    struct mips_got_info *g, *gg;
    const char *name;
+   int idx;
  
    dynobj = elf_hash_table (info)->dynobj;
  
    if (h->plt.offset != MINUS_ONE)
      {
        asection *s;
!       bfd_byte stub[MIPS_FUNCTION_STUB_MAX_SIZE];
  
        /* This symbol has a stub.  Set it up.  */
  
*************** _bfd_mips_elf_finish_dynamic_symbol (bfd
*** 8013,8030 ****
  				   MIPS_ELF_STUB_SECTION_NAME (dynobj));
        BFD_ASSERT (s != NULL);
  
!       /* FIXME: Can h->dynindx be more than 64K?  */
!       if (h->dynindx & 0xffff0000)
  	return FALSE;
  
        /* Fill the stub.  */
!       bfd_put_32 (output_bfd, STUB_LW (output_bfd), stub);
!       bfd_put_32 (output_bfd, STUB_MOVE (output_bfd), stub + 4);
!       bfd_put_32 (output_bfd, STUB_JALR, stub + 8);
!       bfd_put_32 (output_bfd, STUB_LI16 (output_bfd) + h->dynindx, stub + 12);
  
        BFD_ASSERT (h->plt.offset <= s->size);
!       memcpy (s->contents + h->plt.offset, stub, MIPS_FUNCTION_STUB_SIZE);
  
        /* Mark the symbol as undefined.  plt.offset != -1 occurs
  	 only for the referenced symbol.  */
--- 8020,8061 ----
  				   MIPS_ELF_STUB_SECTION_NAME (dynobj));
        BFD_ASSERT (s != NULL);
  
!       BFD_ASSERT ((MIPS_FUNCTION_STUB_SIZE (info) == 20)
!                   || (h->dynindx <= 65536));
! 
!       /* Values up to 2^31 - 1 are allowed.  Larger values would cause
!        * sign extension at runtime in the stub, resulting in a negative
!        * index value.
!        */
!       if (h->dynindx & 0x80000000)
  	return FALSE;
  
        /* Fill the stub.  */
!       idx = 0;
!       bfd_put_32 (output_bfd, STUB_LW (output_bfd), stub + idx);
!       idx += 4;
!       bfd_put_32 (output_bfd, STUB_MOVE (output_bfd), stub + idx);
!       idx += 4;
!       if (MIPS_FUNCTION_STUB_SIZE (info) == 20)
!         {
!           bfd_put_32 (output_bfd, STUB_LUI ((h->dynindx >> 16 ) & 0xffff),
!                       stub + idx);
!           idx += 4;
!         }
!       bfd_put_32 (output_bfd, STUB_JALR, stub + idx);
!       idx += 4;
  
+       /* If a large stub is not required *and* sign extension is not a
+        * problem, then use legacy code in the stub. */
+       if ((MIPS_FUNCTION_STUB_SIZE (info) == 20) || (h->dynindx & 0xffff8000))
+         bfd_put_32 (output_bfd, STUB_LI16U (h->dynindx & 0xffff), stub + idx);
+       else
+         bfd_put_32 (output_bfd,
+                     STUB_LI16S (output_bfd, h->dynindx & 0xffff), stub + idx);
+         
        BFD_ASSERT (h->plt.offset <= s->size);
!       memcpy (s->contents + h->plt.offset,
!               stub, MIPS_FUNCTION_STUB_SIZE (info));
  
        /* Mark the symbol as undefined.  plt.offset != -1 occurs
  	 only for the referenced symbol.  */
*************** _bfd_mips_elf_finish_dynamic_sections (b
*** 8827,8836 ****
  	      {
  		file_ptr dummy_offset;
  
! 		BFD_ASSERT (s->size >= MIPS_FUNCTION_STUB_SIZE);
! 		dummy_offset = s->size - MIPS_FUNCTION_STUB_SIZE;
  		memset (s->contents + dummy_offset, 0,
! 			MIPS_FUNCTION_STUB_SIZE);
  	      }
  	  }
        }
--- 8858,8867 ----
  	      {
  		file_ptr dummy_offset;
  
! 		BFD_ASSERT (s->size >= MIPS_FUNCTION_STUB_SIZE (info));
! 		dummy_offset = s->size - MIPS_FUNCTION_STUB_SIZE (info);
  		memset (s->contents + dummy_offset, 0,
! 			MIPS_FUNCTION_STUB_SIZE (info));
  	      }
  	  }
        }

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-08 23:03 [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols David Daney
@ 2006-06-08 23:12 ` Thiemo Seufer
  2006-06-08 23:19   ` David Daney
  2006-06-09  0:01   ` David Daney
  2006-06-08 23:35 ` Daniel Jacobowitz
  2006-06-09  7:59 ` Richard Sandiford
  2 siblings, 2 replies; 28+ messages in thread
From: Thiemo Seufer @ 2006-06-08 23:12 UTC (permalink / raw)
  To: David Daney; +Cc: binutils, Richard Sandiford, Daniel Jacobowitz

David Daney wrote:
[snip]
> Tested mipsel-linux cross build running on i686-pc-linux-gnu with make 
> -k check showing no regressions.  Also tested that java executables from 
> GCC-4.2 now are able to run.  Small objects still have the original 16 
> byte stub, libgcj.so.7.0.0 uses the 20 byte stub.

I'm not going to demand it, but a testcase would be a good thing.

> Perhaps others could test on mips64 and/or big-endian systems.
> 
> OK to commit?
> 
> I know it is a bit late in the game, but how about for 2.17?  It would 
> be nice to say that gcc-4.2 can be used with an official binutils release.

Would be great, but that's Daniel's call. :-)

[snip]
> --- 8020,8061 ----
>   				   MIPS_ELF_STUB_SECTION_NAME (dynobj));
>         BFD_ASSERT (s != NULL);
>   
> !       BFD_ASSERT ((MIPS_FUNCTION_STUB_SIZE (info) == 20)
> !                   || (h->dynindx <= 65536));
> ! 
> !       /* Values up to 2^31 - 1 are allowed.  Larger values would cause
> !        * sign extension at runtime in the stub, resulting in a negative
> !        * index value.
> !        */
> !       if (h->dynindx & 0x80000000)
>   	return FALSE;
>   
>         /* Fill the stub.  */
> !       idx = 0;
> !       bfd_put_32 (output_bfd, STUB_LW (output_bfd), stub + idx);
> !       idx += 4;
> !       bfd_put_32 (output_bfd, STUB_MOVE (output_bfd), stub + idx);
> !       idx += 4;
> !       if (MIPS_FUNCTION_STUB_SIZE (info) == 20)
> !         {
> !           bfd_put_32 (output_bfd, STUB_LUI ((h->dynindx >> 16 ) & 0xffff),

Formatting. Otherwise ok for trunk.


Thiemo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more  than 2^15 symbols.
  2006-06-08 23:12 ` Thiemo Seufer
@ 2006-06-08 23:19   ` David Daney
  2006-06-09  1:16     ` Thiemo Seufer
  2006-06-09  0:01   ` David Daney
  1 sibling, 1 reply; 28+ messages in thread
From: David Daney @ 2006-06-08 23:19 UTC (permalink / raw)
  To: Thiemo Seufer; +Cc: binutils

Thiemo Seufer wrote:
> David Daney wrote:
>>--- 8020,8061 ----
>>  				   MIPS_ELF_STUB_SECTION_NAME (dynobj));
>>        BFD_ASSERT (s != NULL);
>>  
>>!       BFD_ASSERT ((MIPS_FUNCTION_STUB_SIZE (info) == 20)
>>!                   || (h->dynindx <= 65536));
>>! 
>>!       /* Values up to 2^31 - 1 are allowed.  Larger values would cause
>>!        * sign extension at runtime in the stub, resulting in a negative
>>!        * index value.
>>!        */
>>!       if (h->dynindx & 0x80000000)
>>  	return FALSE;
>>  
>>        /* Fill the stub.  */
>>!       idx = 0;
>>!       bfd_put_32 (output_bfd, STUB_LW (output_bfd), stub + idx);
>>!       idx += 4;
>>!       bfd_put_32 (output_bfd, STUB_MOVE (output_bfd), stub + idx);
>>!       idx += 4;
>>!       if (MIPS_FUNCTION_STUB_SIZE (info) == 20)
>>!         {
>>!           bfd_put_32 (output_bfd, STUB_LUI ((h->dynindx >> 16 ) & 0xffff),
> 
> 
> Formatting. Otherwise ok for trunk.
> 

I try to keep current on GNU coding standards, but could you tell me 
which aspect of the formatting is non-compliant?

David Daney.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-08 23:03 [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols David Daney
  2006-06-08 23:12 ` Thiemo Seufer
@ 2006-06-08 23:35 ` Daniel Jacobowitz
  2006-06-09  7:59 ` Richard Sandiford
  2 siblings, 0 replies; 28+ messages in thread
From: Daniel Jacobowitz @ 2006-06-08 23:35 UTC (permalink / raw)
  To: David Daney; +Cc: binutils, Richard Sandiford, Thiemo Seufer

On Thu, Jun 08, 2006 at 03:20:13PM -0700, David Daney wrote:
> +       /* If a large stub is not required *and* sign extension is not a
> +        * problem, then use legacy code in the stub. */

In addition to what Thiemo mentioned, no star on the second line, and
two spaces after the period.

Otherwise, when this is committed for trunk, please commit it to the
branch also.

-- 
Daniel Jacobowitz
CodeSourcery

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more  than 2^15 symbols.
  2006-06-08 23:12 ` Thiemo Seufer
  2006-06-08 23:19   ` David Daney
@ 2006-06-09  0:01   ` David Daney
  2006-06-09  6:38     ` David Daney
  1 sibling, 1 reply; 28+ messages in thread
From: David Daney @ 2006-06-09  0:01 UTC (permalink / raw)
  To: binutils; +Cc: Thiemo Seufer

[-- Attachment #1: Type: text/plain, Size: 637 bytes --]

Thiemo Seufer wrote:
> 
> Formatting. Otherwise ok for trunk.
> 

Committed this version:

2006-06-08  David Daney  <ddaney@avtrex.com>

	* elfxx-mips.c (STUB_LI16): Removed.
	(STUB_LUI): New macro.
	(STUB_LI16U): Ditto.
	(STUB_LI16S): Ditto.
	(MIPS_FUNCTION_STUB_SIZE): Rewrote to take info parameter.
	(_bfd_mips_elf_adjust_dynamic_symbol): Pass info parameter to
	MIPS_FUNCTION_STUB_SIZE.
	(_bfd_mips_elf_always_size_sections): Ditto.
	(_bfd_mips_elf_size_dynamic_sections): Ditto.
	(_bfd_mips_elf_finish_dynamic_sections): Ditto.
	(_bfd_mips_elf_finish_dynamic_symbol): Rewrote stub generation
	to allow larger symbol table indexes.

[-- Attachment #2: mips-stubs.diff.txt --]
[-- Type: text/plain, Size: 8963 bytes --]

? doc/bfd.info
Index: ChangeLog
===================================================================
RCS file: /cvs/src/src/bfd/ChangeLog,v
retrieving revision 1.3541
diff -c -p -r1.3541 ChangeLog
*** ChangeLog	7 Jun 2006 15:38:00 -0000	1.3541
--- ChangeLog	8 Jun 2006 23:50:16 -0000
***************
*** 1,3 ****
--- 1,18 ----
+ 2006-06-08  David Daney  <ddaney@avtrex.com>
+ 
+ 	* elfxx-mips.c (STUB_LI16): Removed.
+ 	(STUB_LUI): New macro.
+ 	(STUB_LI16U): Ditto.
+ 	(STUB_LI16S): Ditto.
+ 	(MIPS_FUNCTION_STUB_SIZE): Rewrote to take info parameter.
+ 	(_bfd_mips_elf_adjust_dynamic_symbol): Pass info parameter to
+ 	MIPS_FUNCTION_STUB_SIZE.
+ 	(_bfd_mips_elf_always_size_sections): Ditto.
+ 	(_bfd_mips_elf_size_dynamic_sections): Ditto.
+ 	(_bfd_mips_elf_finish_dynamic_sections): Ditto.
+ 	(_bfd_mips_elf_finish_dynamic_symbol): Rewrote stub generation
+ 	to allow larger symbol table indexes.
+ 
  2006-06-07  Joseph S. Myers  <joseph@codesourcery.com>
  
  	* po/Make-in (pdf, ps): New dummy targets.
Index: elfxx-mips.c
===================================================================
RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
retrieving revision 1.169
diff -c -p -r1.169 elfxx-mips.c
*** elfxx-mips.c	22 May 2006 15:06:22 -0000	1.169
--- elfxx-mips.c	8 Jun 2006 23:50:20 -0000
*************** static bfd *reldyn_sorting_bfd;
*** 623,642 ****
  #define MIPS_ELF_GOT_MAX_SIZE(INFO) (ELF_MIPS_GP_OFFSET (INFO) + 0x7fff)
  
  /* Instructions which appear in a stub.  */
! #define STUB_LW(abfd)						\
!   ((ABI_64_P (abfd)  						\
!     ? 0xdf998010		/* ld t9,0x8010(gp) */		\
!     : 0x8f998010))              /* lw t9,0x8010(gp) */
! #define STUB_MOVE(abfd)                                         \
!    ((ABI_64_P (abfd)						\
!      ? 0x03e0782d		/* daddu t7,ra */		\
!      : 0x03e07821))		/* addu t7,ra */
! #define STUB_JALR 0x0320f809	/* jalr t9,ra */
! #define STUB_LI16(abfd)                                         \
!   ((ABI_64_P (abfd)						\
!    ? 0x64180000			/* daddiu t8,zero,0 */		\
!    : 0x24180000))		/* addiu t8,zero,0 */
! #define MIPS_FUNCTION_STUB_SIZE (16)
  
  /* The name of the dynamic interpreter.  This is put in the .interp
     section.  */
--- 623,648 ----
  #define MIPS_ELF_GOT_MAX_SIZE(INFO) (ELF_MIPS_GP_OFFSET (INFO) + 0x7fff)
  
  /* Instructions which appear in a stub.  */
! #define STUB_LW(abfd)							\
!   ((ABI_64_P (abfd)							\
!     ? 0xdf998010				/* ld t9,0x8010(gp) */	\
!     : 0x8f998010))              		/* lw t9,0x8010(gp) */
! #define STUB_MOVE(abfd)							\
!    ((ABI_64_P (abfd)							\
!      ? 0x03e0782d				/* daddu t7,ra */	\
!      : 0x03e07821))				/* addu t7,ra */
! #define STUB_LUI(VAL) (0x3c180000 + (VAL))	/* lui t8,VAL */
! #define STUB_JALR 0x0320f809			/* jalr t9,ra */
! #define STUB_LI16U(VAL) (0x34180000 + (VAL))	/* ori t8,zero,VAL unsigned*/
! #define STUB_LI16S(abfd, VAL)						\
!    ((ABI_64_P (abfd)							\
!     ? (0x64180000 + (VAL))	/* daddiu t8,zero,VAL sign extended */	\
!     : (0x24180000 + (VAL))))	/* addiu t8,zero,VAL sign extended */
! 
! #define MIPS_FUNCTION_STUB_SIZE(INFO) \
!   (elf_hash_table (INFO)->dynsymcount > 65536 ? 20 : 16)
! 
! #define MIPS_FUNCTION_STUB_MAX_SIZE 20
  
  /* The name of the dynamic interpreter.  This is put in the .interp
     section.  */
*************** _bfd_mips_elf_adjust_dynamic_symbol (str
*** 6877,6883 ****
  	  h->plt.offset = s->size;
  
  	  /* Make room for this stub code.  */
! 	  s->size += MIPS_FUNCTION_STUB_SIZE;
  
  	  /* The last half word of the stub will be filled with the index
  	     of this symbol in .dynsym section.  */
--- 6883,6889 ----
  	  h->plt.offset = s->size;
  
  	  /* Make room for this stub code.  */
! 	  s->size += MIPS_FUNCTION_STUB_SIZE (info);
  
  	  /* The last half word of the stub will be filled with the index
  	     of this symbol in .dynsym section.  */
*************** _bfd_mips_elf_always_size_sections (bfd 
*** 7142,7148 ****
    /* In the worst case, we'll get one stub per dynamic symbol, plus
       one to account for the dummy entry at the end required by IRIX
       rld.  */
!   loadable_size += MIPS_FUNCTION_STUB_SIZE * (i + 1);
  
    if (htab->is_vxworks)
      /* There's no need to allocate page entries for VxWorks; R_MIPS_GOT16
--- 7148,7154 ----
    /* In the worst case, we'll get one stub per dynamic symbol, plus
       one to account for the dummy entry at the end required by IRIX
       rld.  */
!   loadable_size += MIPS_FUNCTION_STUB_SIZE (info) * (i + 1);
  
    if (htab->is_vxworks)
      /* There's no need to allocate page entries for VxWorks; R_MIPS_GOT16
*************** _bfd_mips_elf_size_dynamic_sections (bfd
*** 7360,7366 ****
  	{
  	  /* IRIX rld assumes that the function stub isn't at the end
  	     of .text section. So put a dummy. XXX  */
! 	  s->size += MIPS_FUNCTION_STUB_SIZE;
  	}
        else if (! info->shared
  	       && ! mips_elf_hash_table (info)->use_rld_obj_head
--- 7366,7372 ----
  	{
  	  /* IRIX rld assumes that the function stub isn't at the end
  	     of .text section. So put a dummy. XXX  */
! 	  s->size += MIPS_FUNCTION_STUB_SIZE (info);
  	}
        else if (! info->shared
  	       && ! mips_elf_hash_table (info)->use_rld_obj_head
*************** _bfd_mips_elf_finish_dynamic_symbol (bfd
*** 7997,8009 ****
    asection *sgot;
    struct mips_got_info *g, *gg;
    const char *name;
  
    dynobj = elf_hash_table (info)->dynobj;
  
    if (h->plt.offset != MINUS_ONE)
      {
        asection *s;
!       bfd_byte stub[MIPS_FUNCTION_STUB_SIZE];
  
        /* This symbol has a stub.  Set it up.  */
  
--- 8003,8016 ----
    asection *sgot;
    struct mips_got_info *g, *gg;
    const char *name;
+   int idx;
  
    dynobj = elf_hash_table (info)->dynobj;
  
    if (h->plt.offset != MINUS_ONE)
      {
        asection *s;
!       bfd_byte stub[MIPS_FUNCTION_STUB_MAX_SIZE];
  
        /* This symbol has a stub.  Set it up.  */
  
*************** _bfd_mips_elf_finish_dynamic_symbol (bfd
*** 8013,8030 ****
  				   MIPS_ELF_STUB_SECTION_NAME (dynobj));
        BFD_ASSERT (s != NULL);
  
!       /* FIXME: Can h->dynindx be more than 64K?  */
!       if (h->dynindx & 0xffff0000)
  	return FALSE;
  
        /* Fill the stub.  */
!       bfd_put_32 (output_bfd, STUB_LW (output_bfd), stub);
!       bfd_put_32 (output_bfd, STUB_MOVE (output_bfd), stub + 4);
!       bfd_put_32 (output_bfd, STUB_JALR, stub + 8);
!       bfd_put_32 (output_bfd, STUB_LI16 (output_bfd) + h->dynindx, stub + 12);
  
        BFD_ASSERT (h->plt.offset <= s->size);
!       memcpy (s->contents + h->plt.offset, stub, MIPS_FUNCTION_STUB_SIZE);
  
        /* Mark the symbol as undefined.  plt.offset != -1 occurs
  	 only for the referenced symbol.  */
--- 8020,8060 ----
  				   MIPS_ELF_STUB_SECTION_NAME (dynobj));
        BFD_ASSERT (s != NULL);
  
!       BFD_ASSERT ((MIPS_FUNCTION_STUB_SIZE (info) == 20)
!                   || (h->dynindx <= 65536));
! 
!       /* Values up to 2^31 - 1 are allowed.  Larger values would cause
!          sign extension at runtime in the stub, resulting in a
!          negative index value.  */
!       if (h->dynindx & 0x80000000)
  	return FALSE;
  
        /* Fill the stub.  */
!       idx = 0;
!       bfd_put_32 (output_bfd, STUB_LW (output_bfd), stub + idx);
!       idx += 4;
!       bfd_put_32 (output_bfd, STUB_MOVE (output_bfd), stub + idx);
!       idx += 4;
!       if (MIPS_FUNCTION_STUB_SIZE (info) == 20)
!         {
!           bfd_put_32 (output_bfd, STUB_LUI ((h->dynindx >> 16 ) & 0xffff),
!                       stub + idx);
!           idx += 4;
!         }
!       bfd_put_32 (output_bfd, STUB_JALR, stub + idx);
!       idx += 4;
  
+       /* If a large stub is not required and sign extension is not a
+          problem, then use legacy code in the stub.  */
+       if ((MIPS_FUNCTION_STUB_SIZE (info) == 20) || (h->dynindx & 0xffff8000))
+         bfd_put_32 (output_bfd, STUB_LI16U (h->dynindx & 0xffff), stub + idx);
+       else
+         bfd_put_32 (output_bfd,
+                     STUB_LI16S (output_bfd, h->dynindx & 0xffff), stub + idx);
+         
        BFD_ASSERT (h->plt.offset <= s->size);
!       memcpy (s->contents + h->plt.offset,
!               stub, MIPS_FUNCTION_STUB_SIZE (info));
  
        /* Mark the symbol as undefined.  plt.offset != -1 occurs
  	 only for the referenced symbol.  */
*************** _bfd_mips_elf_finish_dynamic_sections (b
*** 8827,8836 ****
  	      {
  		file_ptr dummy_offset;
  
! 		BFD_ASSERT (s->size >= MIPS_FUNCTION_STUB_SIZE);
! 		dummy_offset = s->size - MIPS_FUNCTION_STUB_SIZE;
  		memset (s->contents + dummy_offset, 0,
! 			MIPS_FUNCTION_STUB_SIZE);
  	      }
  	  }
        }
--- 8857,8866 ----
  	      {
  		file_ptr dummy_offset;
  
! 		BFD_ASSERT (s->size >= MIPS_FUNCTION_STUB_SIZE (info));
! 		dummy_offset = s->size - MIPS_FUNCTION_STUB_SIZE (info);
  		memset (s->contents + dummy_offset, 0,
! 			MIPS_FUNCTION_STUB_SIZE (info));
  	      }
  	  }
        }

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-08 23:19   ` David Daney
@ 2006-06-09  1:16     ` Thiemo Seufer
  0 siblings, 0 replies; 28+ messages in thread
From: Thiemo Seufer @ 2006-06-09  1:16 UTC (permalink / raw)
  To: David Daney; +Cc: binutils

David Daney wrote:
> Thiemo Seufer wrote:
> >David Daney wrote:
> >>--- 8020,8061 ----
> >> 				   MIPS_ELF_STUB_SECTION_NAME (dynobj));
> >>       BFD_ASSERT (s != NULL);
> >> 
> >>!       BFD_ASSERT ((MIPS_FUNCTION_STUB_SIZE (info) == 20)
> >>!                   || (h->dynindx <= 65536));
> >>! 
> >>!       /* Values up to 2^31 - 1 are allowed.  Larger values would cause
> >>!        * sign extension at runtime in the stub, resulting in a negative
> >>!        * index value.
> >>!        */
> >>!       if (h->dynindx & 0x80000000)
> >> 	return FALSE;
> >> 
> >>       /* Fill the stub.  */
> >>!       idx = 0;
> >>!       bfd_put_32 (output_bfd, STUB_LW (output_bfd), stub + idx);
> >>!       idx += 4;
> >>!       bfd_put_32 (output_bfd, STUB_MOVE (output_bfd), stub + idx);
> >>!       idx += 4;
> >>!       if (MIPS_FUNCTION_STUB_SIZE (info) == 20)
> >>!         {
> >>!           bfd_put_32 (output_bfd, STUB_LUI ((h->dynindx >> 16 ) & 
> >>0xffff),
> >
> >
> >Formatting. Otherwise ok for trunk.
> >
> 
> I try to keep current on GNU coding standards, but could you tell me 
> which aspect of the formatting is non-compliant?

The dangling closing parenthesis. And, as Daniel noted, the comment
formatting.


Thiemo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more   than 2^15 symbols.
  2006-06-09  0:01   ` David Daney
@ 2006-06-09  6:38     ` David Daney
  0 siblings, 0 replies; 28+ messages in thread
From: David Daney @ 2006-06-09  6:38 UTC (permalink / raw)
  To: binutils; +Cc: Daniel Jacobowitz

David Daney wrote:
> Thiemo Seufer wrote:
> 
>>
>> Formatting. Otherwise ok for trunk.
>>
> 
> Committed this version:
> 
> 2006-06-08  David Daney  <ddaney@avtrex.com>
> 
>     * elfxx-mips.c (STUB_LI16): Removed.
>     (STUB_LUI): New macro.
>     (STUB_LI16U): Ditto.
>     (STUB_LI16S): Ditto.
>     (MIPS_FUNCTION_STUB_SIZE): Rewrote to take info parameter.
>     (_bfd_mips_elf_adjust_dynamic_symbol): Pass info parameter to
>     MIPS_FUNCTION_STUB_SIZE.
>     (_bfd_mips_elf_always_size_sections): Ditto.
>     (_bfd_mips_elf_size_dynamic_sections): Ditto.
>     (_bfd_mips_elf_finish_dynamic_sections): Ditto.
>     (_bfd_mips_elf_finish_dynamic_symbol): Rewrote stub generation
>     to allow larger symbol table indexes.
> 
> 

I just commited (after retesting) this same patch to 
binutils-2_17-branch, there was an offset of 6 lines.

David Daney


> ------------------------------------------------------------------------
> 
> ? doc/bfd.info
> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/src/src/bfd/ChangeLog,v
> retrieving revision 1.3541
> diff -c -p -r1.3541 ChangeLog
> *** ChangeLog	7 Jun 2006 15:38:00 -0000	1.3541
> --- ChangeLog	8 Jun 2006 23:50:16 -0000
> ***************
> *** 1,3 ****
> --- 1,18 ----
> + 2006-06-08  David Daney  <ddaney@avtrex.com>
> + 
> + 	* elfxx-mips.c (STUB_LI16): Removed.
> + 	(STUB_LUI): New macro.
> + 	(STUB_LI16U): Ditto.
> + 	(STUB_LI16S): Ditto.
> + 	(MIPS_FUNCTION_STUB_SIZE): Rewrote to take info parameter.
> + 	(_bfd_mips_elf_adjust_dynamic_symbol): Pass info parameter to
> + 	MIPS_FUNCTION_STUB_SIZE.
> + 	(_bfd_mips_elf_always_size_sections): Ditto.
> + 	(_bfd_mips_elf_size_dynamic_sections): Ditto.
> + 	(_bfd_mips_elf_finish_dynamic_sections): Ditto.
> + 	(_bfd_mips_elf_finish_dynamic_symbol): Rewrote stub generation
> + 	to allow larger symbol table indexes.
> + 
>   2006-06-07  Joseph S. Myers  <joseph@codesourcery.com>
>   
>   	* po/Make-in (pdf, ps): New dummy targets.
> Index: elfxx-mips.c
> ===================================================================
> RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
> retrieving revision 1.169
> diff -c -p -r1.169 elfxx-mips.c
> *** elfxx-mips.c	22 May 2006 15:06:22 -0000	1.169
> --- elfxx-mips.c	8 Jun 2006 23:50:20 -0000
> *************** static bfd *reldyn_sorting_bfd;
> *** 623,642 ****
>   #define MIPS_ELF_GOT_MAX_SIZE(INFO) (ELF_MIPS_GP_OFFSET (INFO) + 0x7fff)
>   
>   /* Instructions which appear in a stub.  */
> ! #define STUB_LW(abfd)						\
> !   ((ABI_64_P (abfd)  						\
> !     ? 0xdf998010		/* ld t9,0x8010(gp) */		\
> !     : 0x8f998010))              /* lw t9,0x8010(gp) */
> ! #define STUB_MOVE(abfd)                                         \
> !    ((ABI_64_P (abfd)						\
> !      ? 0x03e0782d		/* daddu t7,ra */		\
> !      : 0x03e07821))		/* addu t7,ra */
> ! #define STUB_JALR 0x0320f809	/* jalr t9,ra */
> ! #define STUB_LI16(abfd)                                         \
> !   ((ABI_64_P (abfd)						\
> !    ? 0x64180000			/* daddiu t8,zero,0 */		\
> !    : 0x24180000))		/* addiu t8,zero,0 */
> ! #define MIPS_FUNCTION_STUB_SIZE (16)
>   
>   /* The name of the dynamic interpreter.  This is put in the .interp
>      section.  */
> --- 623,648 ----
>   #define MIPS_ELF_GOT_MAX_SIZE(INFO) (ELF_MIPS_GP_OFFSET (INFO) + 0x7fff)
>   
>   /* Instructions which appear in a stub.  */
> ! #define STUB_LW(abfd)							\
> !   ((ABI_64_P (abfd)							\
> !     ? 0xdf998010				/* ld t9,0x8010(gp) */	\
> !     : 0x8f998010))              		/* lw t9,0x8010(gp) */
> ! #define STUB_MOVE(abfd)							\
> !    ((ABI_64_P (abfd)							\
> !      ? 0x03e0782d				/* daddu t7,ra */	\
> !      : 0x03e07821))				/* addu t7,ra */
> ! #define STUB_LUI(VAL) (0x3c180000 + (VAL))	/* lui t8,VAL */
> ! #define STUB_JALR 0x0320f809			/* jalr t9,ra */
> ! #define STUB_LI16U(VAL) (0x34180000 + (VAL))	/* ori t8,zero,VAL unsigned*/
> ! #define STUB_LI16S(abfd, VAL)						\
> !    ((ABI_64_P (abfd)							\
> !     ? (0x64180000 + (VAL))	/* daddiu t8,zero,VAL sign extended */	\
> !     : (0x24180000 + (VAL))))	/* addiu t8,zero,VAL sign extended */
> ! 
> ! #define MIPS_FUNCTION_STUB_SIZE(INFO) \
> !   (elf_hash_table (INFO)->dynsymcount > 65536 ? 20 : 16)
> ! 
> ! #define MIPS_FUNCTION_STUB_MAX_SIZE 20
>   
>   /* The name of the dynamic interpreter.  This is put in the .interp
>      section.  */
> *************** _bfd_mips_elf_adjust_dynamic_symbol (str
> *** 6877,6883 ****
>   	  h->plt.offset = s->size;
>   
>   	  /* Make room for this stub code.  */
> ! 	  s->size += MIPS_FUNCTION_STUB_SIZE;
>   
>   	  /* The last half word of the stub will be filled with the index
>   	     of this symbol in .dynsym section.  */
> --- 6883,6889 ----
>   	  h->plt.offset = s->size;
>   
>   	  /* Make room for this stub code.  */
> ! 	  s->size += MIPS_FUNCTION_STUB_SIZE (info);
>   
>   	  /* The last half word of the stub will be filled with the index
>   	     of this symbol in .dynsym section.  */
> *************** _bfd_mips_elf_always_size_sections (bfd 
> *** 7142,7148 ****
>     /* In the worst case, we'll get one stub per dynamic symbol, plus
>        one to account for the dummy entry at the end required by IRIX
>        rld.  */
> !   loadable_size += MIPS_FUNCTION_STUB_SIZE * (i + 1);
>   
>     if (htab->is_vxworks)
>       /* There's no need to allocate page entries for VxWorks; R_MIPS_GOT16
> --- 7148,7154 ----
>     /* In the worst case, we'll get one stub per dynamic symbol, plus
>        one to account for the dummy entry at the end required by IRIX
>        rld.  */
> !   loadable_size += MIPS_FUNCTION_STUB_SIZE (info) * (i + 1);
>   
>     if (htab->is_vxworks)
>       /* There's no need to allocate page entries for VxWorks; R_MIPS_GOT16
> *************** _bfd_mips_elf_size_dynamic_sections (bfd
> *** 7360,7366 ****
>   	{
>   	  /* IRIX rld assumes that the function stub isn't at the end
>   	     of .text section. So put a dummy. XXX  */
> ! 	  s->size += MIPS_FUNCTION_STUB_SIZE;
>   	}
>         else if (! info->shared
>   	       && ! mips_elf_hash_table (info)->use_rld_obj_head
> --- 7366,7372 ----
>   	{
>   	  /* IRIX rld assumes that the function stub isn't at the end
>   	     of .text section. So put a dummy. XXX  */
> ! 	  s->size += MIPS_FUNCTION_STUB_SIZE (info);
>   	}
>         else if (! info->shared
>   	       && ! mips_elf_hash_table (info)->use_rld_obj_head
> *************** _bfd_mips_elf_finish_dynamic_symbol (bfd
> *** 7997,8009 ****
>     asection *sgot;
>     struct mips_got_info *g, *gg;
>     const char *name;
>   
>     dynobj = elf_hash_table (info)->dynobj;
>   
>     if (h->plt.offset != MINUS_ONE)
>       {
>         asection *s;
> !       bfd_byte stub[MIPS_FUNCTION_STUB_SIZE];
>   
>         /* This symbol has a stub.  Set it up.  */
>   
> --- 8003,8016 ----
>     asection *sgot;
>     struct mips_got_info *g, *gg;
>     const char *name;
> +   int idx;
>   
>     dynobj = elf_hash_table (info)->dynobj;
>   
>     if (h->plt.offset != MINUS_ONE)
>       {
>         asection *s;
> !       bfd_byte stub[MIPS_FUNCTION_STUB_MAX_SIZE];
>   
>         /* This symbol has a stub.  Set it up.  */
>   
> *************** _bfd_mips_elf_finish_dynamic_symbol (bfd
> *** 8013,8030 ****
>   				   MIPS_ELF_STUB_SECTION_NAME (dynobj));
>         BFD_ASSERT (s != NULL);
>   
> !       /* FIXME: Can h->dynindx be more than 64K?  */
> !       if (h->dynindx & 0xffff0000)
>   	return FALSE;
>   
>         /* Fill the stub.  */
> !       bfd_put_32 (output_bfd, STUB_LW (output_bfd), stub);
> !       bfd_put_32 (output_bfd, STUB_MOVE (output_bfd), stub + 4);
> !       bfd_put_32 (output_bfd, STUB_JALR, stub + 8);
> !       bfd_put_32 (output_bfd, STUB_LI16 (output_bfd) + h->dynindx, stub + 12);
>   
>         BFD_ASSERT (h->plt.offset <= s->size);
> !       memcpy (s->contents + h->plt.offset, stub, MIPS_FUNCTION_STUB_SIZE);
>   
>         /* Mark the symbol as undefined.  plt.offset != -1 occurs
>   	 only for the referenced symbol.  */
> --- 8020,8060 ----
>   				   MIPS_ELF_STUB_SECTION_NAME (dynobj));
>         BFD_ASSERT (s != NULL);
>   
> !       BFD_ASSERT ((MIPS_FUNCTION_STUB_SIZE (info) == 20)
> !                   || (h->dynindx <= 65536));
> ! 
> !       /* Values up to 2^31 - 1 are allowed.  Larger values would cause
> !          sign extension at runtime in the stub, resulting in a
> !          negative index value.  */
> !       if (h->dynindx & 0x80000000)
>   	return FALSE;
>   
>         /* Fill the stub.  */
> !       idx = 0;
> !       bfd_put_32 (output_bfd, STUB_LW (output_bfd), stub + idx);
> !       idx += 4;
> !       bfd_put_32 (output_bfd, STUB_MOVE (output_bfd), stub + idx);
> !       idx += 4;
> !       if (MIPS_FUNCTION_STUB_SIZE (info) == 20)
> !         {
> !           bfd_put_32 (output_bfd, STUB_LUI ((h->dynindx >> 16 ) & 0xffff),
> !                       stub + idx);
> !           idx += 4;
> !         }
> !       bfd_put_32 (output_bfd, STUB_JALR, stub + idx);
> !       idx += 4;
>   
> +       /* If a large stub is not required and sign extension is not a
> +          problem, then use legacy code in the stub.  */
> +       if ((MIPS_FUNCTION_STUB_SIZE (info) == 20) || (h->dynindx & 0xffff8000))
> +         bfd_put_32 (output_bfd, STUB_LI16U (h->dynindx & 0xffff), stub + idx);
> +       else
> +         bfd_put_32 (output_bfd,
> +                     STUB_LI16S (output_bfd, h->dynindx & 0xffff), stub + idx);
> +         
>         BFD_ASSERT (h->plt.offset <= s->size);
> !       memcpy (s->contents + h->plt.offset,
> !               stub, MIPS_FUNCTION_STUB_SIZE (info));
>   
>         /* Mark the symbol as undefined.  plt.offset != -1 occurs
>   	 only for the referenced symbol.  */
> *************** _bfd_mips_elf_finish_dynamic_sections (b
> *** 8827,8836 ****
>   	      {
>   		file_ptr dummy_offset;
>   
> ! 		BFD_ASSERT (s->size >= MIPS_FUNCTION_STUB_SIZE);
> ! 		dummy_offset = s->size - MIPS_FUNCTION_STUB_SIZE;
>   		memset (s->contents + dummy_offset, 0,
> ! 			MIPS_FUNCTION_STUB_SIZE);
>   	      }
>   	  }
>         }
> --- 8857,8866 ----
>   	      {
>   		file_ptr dummy_offset;
>   
> ! 		BFD_ASSERT (s->size >= MIPS_FUNCTION_STUB_SIZE (info));
> ! 		dummy_offset = s->size - MIPS_FUNCTION_STUB_SIZE (info);
>   		memset (s->contents + dummy_offset, 0,
> ! 			MIPS_FUNCTION_STUB_SIZE (info));
>   	      }
>   	  }
>         }

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-08 23:03 [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols David Daney
  2006-06-08 23:12 ` Thiemo Seufer
  2006-06-08 23:35 ` Daniel Jacobowitz
@ 2006-06-09  7:59 ` Richard Sandiford
  2006-06-09 16:39   ` Thiemo Seufer
  2 siblings, 1 reply; 28+ messages in thread
From: Richard Sandiford @ 2006-06-09  7:59 UTC (permalink / raw)
  To: David Daney; +Cc: binutils, Thiemo Seufer, Daniel Jacobowitz

David Daney <ddaney@avtrex.com> writes:
> ! #define MIPS_FUNCTION_STUB_SIZE(INFO) \
> !   (elf_hash_table (INFO)->dynsymcount > 65536 ? 20 : 16)

Sorry to be a pain, but as I said earlier, I really do think we should
cache the chosen stub size in mips_elf_link_hash_table (and get rid of
this macro entirely).  That will emphasise that always_size_dynamic_sections
is the place that makes the decision, and that it's only safe to use this
value once that function has been called.  I think that will be more robust
and easier to understand in future.

Apart from that, and from Thiemo's and Daniel's comments, this looks
really good to me.  Thanks a lot for doing this!

Richard

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-09  7:59 ` Richard Sandiford
@ 2006-06-09 16:39   ` Thiemo Seufer
  2006-06-09 18:18     ` David Daney
                       ` (2 more replies)
  0 siblings, 3 replies; 28+ messages in thread
From: Thiemo Seufer @ 2006-06-09 16:39 UTC (permalink / raw)
  To: David Daney, binutils, Daniel Jacobowitz, richard

Richard Sandiford wrote:
> David Daney <ddaney@avtrex.com> writes:
> > ! #define MIPS_FUNCTION_STUB_SIZE(INFO) \
> > !   (elf_hash_table (INFO)->dynsymcount > 65536 ? 20 : 16)
> 
> Sorry to be a pain, but as I said earlier, I really do think we should
> cache the chosen stub size in mips_elf_link_hash_table (and get rid of
> this macro entirely).  That will emphasise that always_size_dynamic_sections
> is the place that makes the decision, and that it's only safe to use this
> value once that function has been called.  I think that will be more robust
> and easier to understand in future.
> 
> Apart from that, and from Thiemo's and Daniel's comments, this looks
> really good to me.  Thanks a lot for doing this!

Does this followup patch look ok?


Thiemo


Index: bfd/elfxx-mips.c
===================================================================
RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
retrieving revision 1.171
diff -u -p -r1.171 elfxx-mips.c
--- bfd/elfxx-mips.c	9 Jun 2006 13:47:41 -0000	1.171
+++ bfd/elfxx-mips.c	9 Jun 2006 15:42:47 -0000
@@ -335,6 +335,8 @@ struct mips_elf_link_hash_table
   bfd_vma plt_header_size;
   /* The size of a PLT entry in bytes (VxWorks only).  */
   bfd_vma plt_entry_size;
+  /* The size of a function stub entry in bytes.  */
+  bfd_vma function_stub_size;
 };
 
 #define TLS_RELOC_P(r_type) \
@@ -633,16 +635,14 @@ static bfd *reldyn_sorting_bfd;
      : 0x03e07821))				/* addu t7,ra */
 #define STUB_LUI(VAL) (0x3c180000 + (VAL))	/* lui t8,VAL */
 #define STUB_JALR 0x0320f809			/* jalr t9,ra */
-#define STUB_LI16U(VAL) (0x34180000 + (VAL))	/* ori t8,zero,VAL unsigned*/
+#define STUB_LI16U(VAL) (0x34180000 + (VAL))	/* ori t8,zero,VAL unsigned */
 #define STUB_LI16S(abfd, VAL)						\
    ((ABI_64_P (abfd)							\
     ? (0x64180000 + (VAL))	/* daddiu t8,zero,VAL sign extended */	\
     : (0x24180000 + (VAL))))	/* addiu t8,zero,VAL sign extended */
 
-#define MIPS_FUNCTION_STUB_SIZE(INFO) \
-  (elf_hash_table (INFO)->dynsymcount > 65536 ? 20 : 16)
-
-#define MIPS_FUNCTION_STUB_MAX_SIZE 20
+#define MIPS_FUNCTION_STUB_NORMAL_SIZE 16
+#define MIPS_FUNCTION_STUB_BIG_SIZE 20
 
 /* The name of the dynamic interpreter.  This is put in the .interp
    section.  */
@@ -5930,6 +5930,9 @@ _bfd_mips_elf_create_dynamic_sections (b
 	  || ! bfd_set_section_alignment (abfd, s,
 					  MIPS_ELF_LOG_FILE_ALIGN (abfd)))
 	return FALSE;
+      htab->function_stub_size = (elf_hash_table (info)->dynsymcount > 0xffff
+				  ? MIPS_FUNCTION_STUB_BIG_SIZE
+				  : MIPS_FUNCTION_STUB_NORMAL_SIZE);
     }
 
   if ((IRIX_COMPAT (abfd) == ict_irix5 || IRIX_COMPAT (abfd) == ict_none)
@@ -6832,7 +6835,9 @@ _bfd_mips_elf_adjust_dynamic_symbol (str
   bfd *dynobj;
   struct mips_elf_link_hash_entry *hmips;
   asection *s;
+  struct mips_elf_link_hash_table *htab;
 
+  htab = mips_elf_hash_table (info);
   dynobj = elf_hash_table (info)->dynobj;
 
   /* Make sure we know what is going on here.  */
@@ -6885,7 +6890,7 @@ _bfd_mips_elf_adjust_dynamic_symbol (str
 	  h->plt.offset = s->size;
 
 	  /* Make room for this stub code.  */
-	  s->size += MIPS_FUNCTION_STUB_SIZE (info);
+	  s->size += htab->function_stub_size;
 
 	  /* The last half word of the stub will be filled with the index
 	     of this symbol in .dynsym section.  */
@@ -7150,7 +7155,7 @@ _bfd_mips_elf_always_size_sections (bfd 
   /* In the worst case, we'll get one stub per dynamic symbol, plus
      one to account for the dummy entry at the end required by IRIX
      rld.  */
-  loadable_size += MIPS_FUNCTION_STUB_SIZE (info) * (i + 1);
+  loadable_size += htab->function_stub_size * (i + 1);
 
   if (htab->is_vxworks)
     /* There's no need to allocate page entries for VxWorks; R_MIPS_GOT16
@@ -7367,14 +7372,14 @@ _bfd_mips_elf_size_dynamic_sections (bfd
       else if (strcmp (name, MIPS_ELF_STUB_SECTION_NAME (output_bfd)) == 0)
 	{
 	  /* IRIX rld assumes that the function stub isn't at the end
-	     of .text section. So put a dummy. XXX  */
-	  s->size += MIPS_FUNCTION_STUB_SIZE (info);
+	     of .text section.  So put a dummy.  XXX  */
+		s->size += htab->function_stub_size;
 	}
       else if (! info->shared
 	       && ! mips_elf_hash_table (info)->use_rld_obj_head
 	       && strncmp (name, ".rld_map", 8) == 0)
 	{
-	  /* We add a room for __rld_map. It will be filled in by the
+	  /* We add a room for __rld_map.  It will be filled in by the
 	     rtld to contain a pointer to the _r_debug structure.  */
 	  s->size += 4;
 	}
@@ -8006,13 +8011,15 @@ _bfd_mips_elf_finish_dynamic_symbol (bfd
   struct mips_got_info *g, *gg;
   const char *name;
   int idx;
+  struct mips_elf_link_hash_table *htab;
 
+  htab = mips_elf_hash_table (info);
   dynobj = elf_hash_table (info)->dynobj;
 
   if (h->plt.offset != MINUS_ONE)
     {
       asection *s;
-      bfd_byte stub[MIPS_FUNCTION_STUB_MAX_SIZE];
+      bfd_byte stub[MIPS_FUNCTION_STUB_BIG_SIZE];
 
       /* This symbol has a stub.  Set it up.  */
 
@@ -8022,13 +8029,13 @@ _bfd_mips_elf_finish_dynamic_symbol (bfd
 				   MIPS_ELF_STUB_SECTION_NAME (dynobj));
       BFD_ASSERT (s != NULL);
 
-      BFD_ASSERT ((MIPS_FUNCTION_STUB_SIZE (info) == 20)
-                  || (h->dynindx <= 65536));
+      BFD_ASSERT ((htab->function_stub_size == MIPS_FUNCTION_STUB_BIG_SIZE)
+                  || (h->dynindx <= 0xffff));
 
       /* Values up to 2^31 - 1 are allowed.  Larger values would cause
-         sign extension at runtime in the stub, resulting in a
-         negative index value.  */
-      if (h->dynindx & 0x80000000)
+	 sign extension at runtime in the stub, resulting in a negative
+	 index value.  */
+      if (h->dynindx & ~0x7fffffff)
 	return FALSE;
 
       /* Fill the stub.  */
@@ -8037,9 +8044,9 @@ _bfd_mips_elf_finish_dynamic_symbol (bfd
       idx += 4;
       bfd_put_32 (output_bfd, STUB_MOVE (output_bfd), stub + idx);
       idx += 4;
-      if (MIPS_FUNCTION_STUB_SIZE (info) == 20)
+      if (htab->function_stub_size == MIPS_FUNCTION_STUB_BIG_SIZE)
         {
-          bfd_put_32 (output_bfd, STUB_LUI ((h->dynindx >> 16 ) & 0xffff),
+          bfd_put_32 (output_bfd, STUB_LUI ((h->dynindx >> 16) & 0x7fff),
                       stub + idx);
           idx += 4;
         }
@@ -8048,15 +8055,15 @@ _bfd_mips_elf_finish_dynamic_symbol (bfd
 
       /* If a large stub is not required and sign extension is not a
          problem, then use legacy code in the stub.  */
-      if ((MIPS_FUNCTION_STUB_SIZE (info) == 20) || (h->dynindx & 0xffff8000))
+      if ((htab->function_stub_size == MIPS_FUNCTION_STUB_BIG_SIZE)
+	  || (h->dynindx & ~0x7fff))
         bfd_put_32 (output_bfd, STUB_LI16U (h->dynindx & 0xffff), stub + idx);
       else
-        bfd_put_32 (output_bfd,
-                    STUB_LI16S (output_bfd, h->dynindx & 0xffff), stub + idx);
-        
+        bfd_put_32 (output_bfd, STUB_LI16S (output_bfd, h->dynindx),
+		    stub + idx);
+
       BFD_ASSERT (h->plt.offset <= s->size);
-      memcpy (s->contents + h->plt.offset,
-              stub, MIPS_FUNCTION_STUB_SIZE (info));
+      memcpy (s->contents + h->plt.offset, stub, htab->function_stub_size);
 
       /* Mark the symbol as undefined.  plt.offset != -1 occurs
 	 only for the referenced symbol.  */
@@ -8859,10 +8866,10 @@ _bfd_mips_elf_finish_dynamic_sections (b
 	      {
 		file_ptr dummy_offset;
 
-		BFD_ASSERT (s->size >= MIPS_FUNCTION_STUB_SIZE (info));
-		dummy_offset = s->size - MIPS_FUNCTION_STUB_SIZE (info);
+		BFD_ASSERT (s->size >= htab->function_stub_size);
+		dummy_offset = s->size - htab->function_stub_size;
 		memset (s->contents + dummy_offset, 0,
-			MIPS_FUNCTION_STUB_SIZE (info));
+			htab->function_stub_size);
 	      }
 	  }
       }
@@ -9974,6 +9981,7 @@ _bfd_mips_elf_link_hash_table_create (bf
   ret->splt = NULL;
   ret->plt_header_size = 0;
   ret->plt_entry_size = 0;
+  ret->function_stub_size = 0;
 
   return &ret->root.root;
 }

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more  than 2^15 symbols.
  2006-06-09 16:39   ` Thiemo Seufer
@ 2006-06-09 18:18     ` David Daney
  2006-06-09 18:23       ` Thiemo Seufer
  2006-06-09 18:28     ` Daniel Jacobowitz
  2006-06-09 21:00     ` Richard Sandiford
  2 siblings, 1 reply; 28+ messages in thread
From: David Daney @ 2006-06-09 18:18 UTC (permalink / raw)
  To: Thiemo Seufer; +Cc: binutils, Daniel Jacobowitz, richard

Thiemo Seufer wrote:
> Richard Sandiford wrote:
> 
>>David Daney <ddaney@avtrex.com> writes:
>>
>>>! #define MIPS_FUNCTION_STUB_SIZE(INFO) \
>>>!   (elf_hash_table (INFO)->dynsymcount > 65536 ? 20 : 16)
>>
>>Sorry to be a pain, but as I said earlier, I really do think we should
>>cache the chosen stub size in mips_elf_link_hash_table (and get rid of
>>this macro entirely).  That will emphasise that always_size_dynamic_sections
>>is the place that makes the decision, and that it's only safe to use this
>>value once that function has been called.  I think that will be more robust
>>and easier to understand in future.
>>
>>Apart from that, and from Thiemo's and Daniel's comments, this looks
>>really good to me.  Thanks a lot for doing this!
> 
> 
> Does this followup patch look ok?
>

FWIW, it looks good to me.  Definitly a little cleaner than mine.

One weird thing is that the generated code is different.

My libgcj.so now has 16 byte stubs.  With my version of the patch it 
generated 20 byte stubs.  This means that at the point you are sampling 
info->dynsymcount, it had a different value than when it was sampled in 
my patch.  I was wondering about this because even though my patch 
generated 20 byte stubs, none of the symbol index values in the stubs 
were large enough to require the larger stub.

There must be more symbols that are added to the symbol table after 
_bfd_mips_elf_create_dynamic_sections is called.  Is it safe to assume 
that none of these 'extra' symbols will be referenced by a stub?

With your patch my simple tests with libgcj run successfully.  I did 
however see FAIL: MIPS multi-got-no-shared, which I think passed before 
applying your patch.  These are very brittle tests, so perhaps it is not 
surprising.


David Daney

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-09 18:18     ` David Daney
@ 2006-06-09 18:23       ` Thiemo Seufer
  2006-06-09 19:40         ` David Daney
  0 siblings, 1 reply; 28+ messages in thread
From: Thiemo Seufer @ 2006-06-09 18:23 UTC (permalink / raw)
  To: David Daney; +Cc: binutils, Daniel Jacobowitz, richard

David Daney wrote:
> Thiemo Seufer wrote:
> >Richard Sandiford wrote:
> >
> >>David Daney <ddaney@avtrex.com> writes:
> >>
> >>>! #define MIPS_FUNCTION_STUB_SIZE(INFO) \
> >>>!   (elf_hash_table (INFO)->dynsymcount > 65536 ? 20 : 16)
> >>
> >>Sorry to be a pain, but as I said earlier, I really do think we should
> >>cache the chosen stub size in mips_elf_link_hash_table (and get rid of
> >>this macro entirely).  That will emphasise that 
> >>always_size_dynamic_sections
> >>is the place that makes the decision, and that it's only safe to use this
> >>value once that function has been called.  I think that will be more 
> >>robust
> >>and easier to understand in future.
> >>
> >>Apart from that, and from Thiemo's and Daniel's comments, this looks
> >>really good to me.  Thanks a lot for doing this!
> >
> >
> >Does this followup patch look ok?
> >
> 
> FWIW, it looks good to me.  Definitly a little cleaner than mine.
> 
> One weird thing is that the generated code is different.
> 
> My libgcj.so now has 16 byte stubs.  With my version of the patch it 
> generated 20 byte stubs.  This means that at the point you are sampling 
> info->dynsymcount, it had a different value than when it was sampled in 
> my patch.  I was wondering about this because even though my patch 
> generated 20 byte stubs, none of the symbol index values in the stubs 
> were large enough to require the larger stub.

Some of the overflow checks were off by one bit.

> With your patch my simple tests with libgcj run successfully.  I did 
> however see FAIL: MIPS multi-got-no-shared, which I think passed before 
> applying your patch.  These are very brittle tests, so perhaps it is not 
> surprising.

No, that's since H.J.'s COMMONPAGESIZE patch, I have a fix for that.


Thiemo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-09 16:39   ` Thiemo Seufer
  2006-06-09 18:18     ` David Daney
@ 2006-06-09 18:28     ` Daniel Jacobowitz
  2006-06-09 19:24       ` David Daney
  2006-06-09 21:00     ` Richard Sandiford
  2 siblings, 1 reply; 28+ messages in thread
From: Daniel Jacobowitz @ 2006-06-09 18:28 UTC (permalink / raw)
  To: Thiemo Seufer; +Cc: David Daney, binutils, richard

When you and Richard agree on a patch, please make sure to fix it on
the 2.17 branch also.  Thanks in advance.

On Fri, Jun 09, 2006 at 04:46:20PM +0100, Thiemo Seufer wrote:
> -#define STUB_LI16U(VAL) (0x34180000 + (VAL))	/* ori t8,zero,VAL unsigned*/
> +#define STUB_LI16U(VAL) (0x34180000 + (VAL))	/* ori t8,zero,VAL unsigned */

Shouldn't this be called an ORI16 now?

-- 
Daniel Jacobowitz
CodeSourcery

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more  than 2^15 symbols.
  2006-06-09 18:28     ` Daniel Jacobowitz
@ 2006-06-09 19:24       ` David Daney
  2006-06-09 19:27         ` Paul Koning
  0 siblings, 1 reply; 28+ messages in thread
From: David Daney @ 2006-06-09 19:24 UTC (permalink / raw)
  To: Daniel Jacobowitz; +Cc: Thiemo Seufer, binutils, richard

Daniel Jacobowitz wrote:
> When you and Richard agree on a patch, please make sure to fix it on
> the 2.17 branch also.  Thanks in advance.
> 
> On Fri, Jun 09, 2006 at 04:46:20PM +0100, Thiemo Seufer wrote:
> 
>>-#define STUB_LI16U(VAL) (0x34180000 + (VAL))	/* ori t8,zero,VAL unsigned*/
>>+#define STUB_LI16U(VAL) (0x34180000 + (VAL))	/* ori t8,zero,VAL unsigned */
> 
> 
> Shouldn't this be called an ORI16 now?
> 

FWIW, the dissassembler still calls it 'li'.

David Daney

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more  than 2^15 symbols.
  2006-06-09 19:24       ` David Daney
@ 2006-06-09 19:27         ` Paul Koning
  0 siblings, 0 replies; 28+ messages in thread
From: Paul Koning @ 2006-06-09 19:27 UTC (permalink / raw)
  To: ddaney; +Cc: drow, ths, binutils, richard

>>>>> "David" == David Daney <ddaney@avtrex.com> writes:

 David> Daniel Jacobowitz wrote:
 >> When you and Richard agree on a patch, please make sure to fix it
 >> on the 2.17 branch also.  Thanks in advance.
 >> 
 >> On Fri, Jun 09, 2006 at 04:46:20PM +0100, Thiemo Seufer wrote:
 >> 
 >>> -#define STUB_LI16U(VAL) (0x34180000 + (VAL)) /* ori t8,zero,VAL
 >>> unsigned*/ +#define STUB_LI16U(VAL) (0x34180000 + (VAL)) /* ori
 >>> t8,zero,VAL unsigned */
 >> 
 >> 
 >> Shouldn't this be called an ORI16 now?
 >> 

 David> FWIW, the dissassembler still calls it 'li'.

That makes sense.  ori and addiu are both used in the expansion of the
builtin macro li, depending on the literal value.

	paul

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more  than 2^15 symbols.
  2006-06-09 18:23       ` Thiemo Seufer
@ 2006-06-09 19:40         ` David Daney
  0 siblings, 0 replies; 28+ messages in thread
From: David Daney @ 2006-06-09 19:40 UTC (permalink / raw)
  To: Thiemo Seufer; +Cc: binutils, Daniel Jacobowitz, richard

Thiemo Seufer wrote:
> David Daney wrote:
> 
>>Thiemo Seufer wrote:
>>
>>>Richard Sandiford wrote:
>>>
>>>
>>>>David Daney <ddaney@avtrex.com> writes:
>>>>
>>>>
>>>>>! #define MIPS_FUNCTION_STUB_SIZE(INFO) \
>>>>>!   (elf_hash_table (INFO)->dynsymcount > 65536 ? 20 : 16)
>>>>
>>>>Sorry to be a pain, but as I said earlier, I really do think we should
>>>>cache the chosen stub size in mips_elf_link_hash_table (and get rid of
>>>>this macro entirely).  That will emphasise that 
>>>>always_size_dynamic_sections
>>>>is the place that makes the decision, and that it's only safe to use this
>>>>value once that function has been called.  I think that will be more 
>>>>robust
>>>>and easier to understand in future.
>>>>
>>>>Apart from that, and from Thiemo's and Daniel's comments, this looks
>>>>really good to me.  Thanks a lot for doing this!
>>>
>>>
>>>Does this followup patch look ok?
>>>
>>
>>FWIW, it looks good to me.  Definitly a little cleaner than mine.
>>
>>One weird thing is that the generated code is different.
>>
>>My libgcj.so now has 16 byte stubs.  With my version of the patch it 
>>generated 20 byte stubs.  This means that at the point you are sampling 
>>info->dynsymcount, it had a different value than when it was sampled in 
>>my patch.  I was wondering about this because even though my patch 
>>generated 20 byte stubs, none of the symbol index values in the stubs 
>>were large enough to require the larger stub.
> 
> 
> Some of the overflow checks were off by one bit.

Your checks are identical to mine.  The test for stub size is likewise 
unchanged.

Look at this:

$ mipsel-linux-readelf -S libgcj.so.7.0.0
There are 40 section headers, starting at offset 0x35f82b8:

Section Headers:
   [Nr] Name              Type            Addr     Off    Size   ES Flg 
Lk Inf Al
   [ 0]                   NULL            00000000 000000 000000 00 
  0   0  0
   [ 1] .reginfo          MIPS_REGINFO    000000d4 0000d4 000018 18   A 
  0   0  4
   [ 2] .dynamic          DYNAMIC         000000ec 0000ec 000120 08   A 
  5   0  4
   [ 3] .hash             HASH            0000020c 00020c 069c98 04   A 
  4   0  4
   [ 4] .dynsym           DYNSYM          00069ea4 069ea4 127210 10   A 
  5  17  4
.
.
.

Note the size of the .dynsym section.  According to my calculations it 
contains about 75553 symbols.

$ mipsel-linux-readelf -s -D libgcj.so.7.0.0 | wc
   75539  679837 6082342

$ mipsel-linux-objdump -d -z -j .MIPS.stubs libgcj.so.7.0.0

libgcj.so.7.0.0:     file format elf32-tradlittlemips

Disassembly of section .MIPS.stubs:

01597f30 <.MIPS.stubs>:
  1597f30:       8f998010        lw      t9,-32752(gp)
  1597f34:       03e07821        move    t7,ra
  1597f38:       0320f809        jalr    t9
  1597f3c:       341897cc        li      t8,0x97cc
  1597f40:       8f998010        lw      t9,-32752(gp)
  1597f44:       03e07821        move    t7,ra
  1597f48:       0320f809        jalr    t9
  1597f4c:       341897b1        li      t8,0x97b1
  1597f50:       8f998010        lw      t9,-32752(gp)
  1597f54:       03e07821        move    t7,ra
  1597f58:       0320f809        jalr    t9
  1597f5c:       34189761        li      t8,0x9761
.
.
.


These are 16 bytes stubs, even though there are more than 2^16 dynamic 
symbols.  With my original patch the stubs were 20 bytes, which made 
sense to me.  I don't understand the discrepancy.

David Daney.




> 
> 
>>With your patch my simple tests with libgcj run successfully.  I did 
>>however see FAIL: MIPS multi-got-no-shared, which I think passed before 
>>applying your patch.  These are very brittle tests, so perhaps it is not 
>>surprising.
> 
> 
> No, that's since H.J.'s COMMONPAGESIZE patch, I have a fix for that.
> 
> 
> Thiemo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-09 16:39   ` Thiemo Seufer
  2006-06-09 18:18     ` David Daney
  2006-06-09 18:28     ` Daniel Jacobowitz
@ 2006-06-09 21:00     ` Richard Sandiford
  2006-06-10 16:04       ` Richard Sandiford
  2 siblings, 1 reply; 28+ messages in thread
From: Richard Sandiford @ 2006-06-09 21:00 UTC (permalink / raw)
  To: Thiemo Seufer; +Cc: David Daney, binutils, Daniel Jacobowitz

Thiemo Seufer <ths@networkno.de> writes:
> Richard Sandiford wrote:
>> David Daney <ddaney@avtrex.com> writes:
>> > ! #define MIPS_FUNCTION_STUB_SIZE(INFO) \
>> > !   (elf_hash_table (INFO)->dynsymcount > 65536 ? 20 : 16)
>> 
>> Sorry to be a pain, but as I said earlier, I really do think we should
>> cache the chosen stub size in mips_elf_link_hash_table (and get rid of
>> this macro entirely).  That will emphasise that always_size_dynamic_sections
>> is the place that makes the decision, and that it's only safe to use this
>> value once that function has been called.  I think that will be more robust
>> and easier to understand in future.
>> 
>> Apart from that, and from Thiemo's and Daniel's comments, this looks
>> really good to me.  Thanks a lot for doing this!
>
> Does this followup patch look ok?

Looks good, apart from the fact that you're setting function_stub_size
in _bfd_mips_elf_create_dynamic_sections rather than the suggested
always_size_dynamic_sections.  Was there a particular reason for that?
As David says, create_dynamic_sections is too early; AIUI,
create_dynamic_sections is called as soon as we know we need
dynamic sections, so the value of dynsymcount at that point is
going to be pretty arbitrary.

The reason I suggested always_size_sections is that I think we should
determine the stub entry size and stub section size at the same time.

Thanks for doing this though!  I volunteer to come up with some
testcases tomorrow.

Richard

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-09 21:00     ` Richard Sandiford
@ 2006-06-10 16:04       ` Richard Sandiford
  2006-06-11  0:07         ` Eric Christopher
  2006-06-11  0:13         ` Thiemo Seufer
  0 siblings, 2 replies; 28+ messages in thread
From: Richard Sandiford @ 2006-06-10 16:04 UTC (permalink / raw)
  To: Thiemo Seufer; +Cc: David Daney, binutils, Daniel Jacobowitz

[-- Attachment #1: Type: text/plain, Size: 4825 bytes --]

Richard Sandiford <richard@codesourcery.com> writes:
> Thiemo Seufer <ths@networkno.de> writes:
>> Richard Sandiford wrote:
>>> David Daney <ddaney@avtrex.com> writes:
>>> > ! #define MIPS_FUNCTION_STUB_SIZE(INFO) \
>>> > !   (elf_hash_table (INFO)->dynsymcount > 65536 ? 20 : 16)
>>> 
>>> Sorry to be a pain, but as I said earlier, I really do think we should
>>> cache the chosen stub size in mips_elf_link_hash_table (and get rid of
>>> this macro entirely).  That will emphasise that always_size_dynamic_sections
>>> is the place that makes the decision, and that it's only safe to use this
>>> value once that function has been called.  I think that will be more robust
>>> and easier to understand in future.
>>> 
>>> Apart from that, and from Thiemo's and Daniel's comments, this looks
>>> really good to me.  Thanks a lot for doing this!
>>
>> Does this followup patch look ok?
>
> Looks good, apart from the fact that you're setting function_stub_size
> in _bfd_mips_elf_create_dynamic_sections rather than the suggested
> always_size_dynamic_sections.  Was there a particular reason for that?
> As David says, create_dynamic_sections is too early; AIUI,
> create_dynamic_sections is called as soon as we know we need
> dynamic sections, so the value of dynsymcount at that point is
> going to be pretty arbitrary.
>
> The reason I suggested always_size_sections is that I think we should
> determine the stub entry size and stub section size at the same time.
>
> Thanks for doing this though!  I volunteer to come up with some
> testcases tomorrow.

OK, here are the testcases.  They showed up a couple of problems:

  (1) We were using "li" for the lower half of 32-bit indexes, thus
      cancelling out the "lui"!  Daniel quoted the definition of
      STUB_LI16U and asked about whether it should be "ori", but I think
      there was some confusion.  We don't want to rename STUB_LI16U to
      STUB_ORI -- it's fine as it is -- but we _do_ want a separate
      STUB_ORI for "ori t8,t8,foo" (as opposed to "ori t8,zero,foo").

  (2) dynsymcount doesn't include the local section symbols at the time
      that the MIPS section-sizing code is run.  Although the exact number
      of such symbols isn't known at that stage, we can get a conservative
      estimate by reusing a loop in _bfd_mips_elf_final_link.

      This loop was originally copied from _bfd_elf_link_renumber_dynsyms.
      We should try to reuse code between the two, as the current MIPS
      version is already out-of-sync, but I'd rather do that separately,
      as it touches non-MIPS code.

Because the estimate in (2) is conservative, we can't insist that a file
with 0x10000 dynsyms use 16-byte stubs.  The tests therefore test a file
with 0xfff1 dynsyms instead.

We often seem to need to update these kinds of tests after changes to
target-independent code.  I've tried to make that easier by including
a base_syms variable.  See the comments for details.

I've attached two patches.  The first is relative to Thiemo's patch
from yesterday; the second is a complete patch against mainline.

Tested on mipsel-linux-gnu, mips64-linux-gnu and mipsisa64-elf.
OK to install?

bfd/
	* elfxx-mips.c (mips_elf_link_hash_table): Add function_stub_size.
	(STUB_ORI): New macro.
	(STUB_LI16U): Fix formatting.
	(MIPS_FUNCTION_STUB_SIZE): Delete.
	(MIPS_FUNCTION_STUB_MAX_SIZE): Likewise.
	(MIPS_FUNCTION_STUB_NORMAL_SIZE): New macro.
	(MIPS_FUNCTION_STUB_BIG_SIZE): Likewise.
	(_bfd_mips_elf_adjust_dynamic_symbol): Use htab->function_stub_size
	instead of MIPS_FUNCTION_STUB_SIZE.
	(count_section_dynsyms): New function, split out from
	_bfd_mips_elf_final_link.
	(_bfd_mips_elf_always_size_sections): Get a worst-case estimate
	of the number of dynamic symbols needed and use it to set up
	function_stub_size.  Use function_stub_size rather than
	MIPS_FUNCTION_STUB_SIZE to determine the size of the stub section.
	Use 16-byte stubs for 0x10000 dynamic symbols.
	(_bfd_mips_elf_size_dynamic_sections): Use htab->function_stub_size
	instead of MIPS_FUNCTION_STUB_SIZE.  Fix formatting.
	(_bfd_mips_elf_finish_dynamic_symbol): Likewise.  Change the
	size of the stub buffer from MIPS_FUNCTION_STUB_MAX_SIZE to
	MIPS_FUNCTION_STUB_BIG_SIZE.  Tweak the check for unhandled dynindxes.
	Use MIPS_FUNCTION_STUB_BIG_SIZE rather than a hard-coded 20.
	Use STUB_ORI rather than STUB_LI16U for big stubs.
	(_bfd_mips_elf_link_hash_table_create): Initialize function_stub_size.
	(_bfd_mips_elf_final_link): Use count_section_dynsyms.

ld/testsuite/
	* ld-mips-elf/stub-dynsym-1.s,
	* ld-mips-elf/stub-dynsym-1.ld,
	* ld-mips-elf/stub-dynsym-1-7fff.d,
	* ld-mips-elf/stub-dynsym-1-8000.d,
	* ld-mips-elf/stub-dynsym-1-fff0.d,
	* ld-mips-elf/stub-dynsym-1-10000.d,
	* ld-mips-elf/stub-dynsym-1-2fe80.d: New test.
	* ld-mips-elf/mips-elf.exp: Run it.


[-- Attachment #2: stub-selection-rel-thiemo.diff --]
[-- Type: text/plain, Size: 4721 bytes --]

--- bfd/elfxx-mips.c.thiemo	2006-06-10 10:49:32.000000000 +0100
+++ bfd/elfxx-mips.c	2006-06-10 11:24:44.000000000 +0100
@@ -635,6 +635,7 @@ static bfd *reldyn_sorting_bfd;
      : 0x03e07821))				/* addu t7,ra */
 #define STUB_LUI(VAL) (0x3c180000 + (VAL))	/* lui t8,VAL */
 #define STUB_JALR 0x0320f809			/* jalr t9,ra */
+#define STUB_ORI(VAL) (0x37180000 + (VAL))	/* ori t8,t8,VAL */
 #define STUB_LI16U(VAL) (0x34180000 + (VAL))	/* ori t8,zero,VAL unsigned */
 #define STUB_LI16S(abfd, VAL)						\
    ((ABI_64_P (abfd)							\
@@ -5930,9 +5931,6 @@ _bfd_mips_elf_create_dynamic_sections (b
 	  || ! bfd_set_section_alignment (abfd, s,
 					  MIPS_ELF_LOG_FILE_ALIGN (abfd)))
 	return FALSE;
-      htab->function_stub_size = (elf_hash_table (info)->dynsymcount > 0xffff
-				  ? MIPS_FUNCTION_STUB_BIG_SIZE
-				  : MIPS_FUNCTION_STUB_NORMAL_SIZE);
     }
 
   if ((IRIX_COMPAT (abfd) == ict_irix5 || IRIX_COMPAT (abfd) == ict_none)
@@ -7078,6 +7076,32 @@ _bfd_mips_vxworks_adjust_dynamic_symbol 
   return TRUE;
 }
 \f
+/* Return the number of dynamic section symbols required by OUTPUT_BFD.
+   The number might be exact or a worst-case estimate, depending on how
+   much information is available to elf_backend_omit_section_dynsym at
+   the current linking stage.  */
+
+static bfd_size_type
+count_section_dynsyms (bfd *output_bfd, struct bfd_link_info *info)
+{
+  bfd_size_type count;
+
+  count = 0;
+  if (info->shared || elf_hash_table (info)->is_relocatable_executable)
+    {
+      asection *p;
+      const struct elf_backend_data *bed;
+
+      bed = get_elf_backend_data (output_bfd);
+      for (p = output_bfd->sections; p ; p = p->next)
+	if ((p->flags & SEC_EXCLUDE) == 0
+	    && (p->flags & SEC_ALLOC) != 0
+	    && !(*bed->elf_backend_omit_section_dynsym) (output_bfd, info, p))
+	  ++count;
+    }
+  return count;
+}
+
 /* This function is called after all the input files have been read,
    and the input sections have been assigned to output sections.  We
    check for any mips16 stub sections that we can discard.  */
@@ -7094,6 +7118,7 @@ _bfd_mips_elf_always_size_sections (bfd 
   int i;
   bfd_size_type loadable_size = 0;
   bfd_size_type local_gotno;
+  bfd_size_type dynsymcount;
   bfd *sub;
   struct mips_elf_count_tls_arg count_tls_arg;
   struct mips_elf_link_hash_table *htab;
@@ -7152,6 +7177,18 @@ _bfd_mips_elf_always_size_sections (bfd 
        relocations, then GLOBAL_GOTSYM will be NULL.  */
     i = 0;
 
+  /* Get a worst-case estimate of the number of dynamic symbols needed.
+     At this point, dynsymcount does not account for section symbols
+     and count_section_dynsyms may overestimate the number that will
+     be needed.  */
+  dynsymcount = (elf_hash_table (info)->dynsymcount
+		 + count_section_dynsyms (output_bfd, info));
+
+  /* Determine the size of one stub entry.  */
+  htab->function_stub_size = (dynsymcount > 0x10000
+			      ? MIPS_FUNCTION_STUB_BIG_SIZE
+			      : MIPS_FUNCTION_STUB_NORMAL_SIZE);
+
   /* In the worst case, we'll get one stub per dynamic symbol, plus
      one to account for the dummy entry at the end required by IRIX
      rld.  */
@@ -7373,7 +7410,7 @@ _bfd_mips_elf_size_dynamic_sections (bfd
 	{
 	  /* IRIX rld assumes that the function stub isn't at the end
 	     of .text section.  So put a dummy.  XXX  */
-		s->size += htab->function_stub_size;
+	  s->size += htab->function_stub_size;
 	}
       else if (! info->shared
 	       && ! mips_elf_hash_table (info)->use_rld_obj_head
@@ -8055,8 +8092,9 @@ _bfd_mips_elf_finish_dynamic_symbol (bfd
 
       /* If a large stub is not required and sign extension is not a
          problem, then use legacy code in the stub.  */
-      if ((htab->function_stub_size == MIPS_FUNCTION_STUB_BIG_SIZE)
-	  || (h->dynindx & ~0x7fff))
+      if (htab->function_stub_size == MIPS_FUNCTION_STUB_BIG_SIZE)
+	bfd_put_32 (output_bfd, STUB_ORI (h->dynindx & 0xffff), stub + idx);
+      else if (h->dynindx & ~0x7fff)
         bfd_put_32 (output_bfd, STUB_LI16U (h->dynindx & 0xffff), stub + idx);
       else
         bfd_put_32 (output_bfd, STUB_LI16S (output_bfd, h->dynindx),
@@ -10057,18 +10095,7 @@ _bfd_mips_elf_final_link (bfd *abfd, str
 	 we count the sections after (possibly) removing the .options
 	 section above.  */
 
-      dynsecsymcount = 0;
-      if (info->shared)
-	{
-	  asection * p;
-
-	  for (p = abfd->sections; p ; p = p->next)
-	    if ((p->flags & SEC_EXCLUDE) == 0
-		&& (p->flags & SEC_ALLOC) != 0
-		&& !(*bed->elf_backend_omit_section_dynsym) (abfd, info, p))
-	      ++ dynsecsymcount;
-	}
-
+      dynsecsymcount = count_section_dynsyms (abfd, info);
       if (! mips_elf_sort_hash_table (info, dynsecsymcount + 1))
 	return FALSE;
 

[-- Attachment #3: stub-selection.diff --]
[-- Type: text/plain, Size: 15531 bytes --]

Index: bfd/elfxx-mips.c
===================================================================
RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
retrieving revision 1.171
diff -u -p -r1.171 elfxx-mips.c
--- bfd/elfxx-mips.c	9 Jun 2006 13:47:41 -0000	1.171
+++ bfd/elfxx-mips.c	10 Jun 2006 10:16:53 -0000
@@ -335,6 +335,8 @@ struct mips_elf_link_hash_table
   bfd_vma plt_header_size;
   /* The size of a PLT entry in bytes (VxWorks only).  */
   bfd_vma plt_entry_size;
+  /* The size of a function stub entry in bytes.  */
+  bfd_vma function_stub_size;
 };
 
 #define TLS_RELOC_P(r_type) \
@@ -633,16 +635,15 @@ static bfd *reldyn_sorting_bfd;
      : 0x03e07821))				/* addu t7,ra */
 #define STUB_LUI(VAL) (0x3c180000 + (VAL))	/* lui t8,VAL */
 #define STUB_JALR 0x0320f809			/* jalr t9,ra */
-#define STUB_LI16U(VAL) (0x34180000 + (VAL))	/* ori t8,zero,VAL unsigned*/
+#define STUB_ORI(VAL) (0x37180000 + (VAL))	/* ori t8,t8,VAL */
+#define STUB_LI16U(VAL) (0x34180000 + (VAL))	/* ori t8,zero,VAL unsigned */
 #define STUB_LI16S(abfd, VAL)						\
    ((ABI_64_P (abfd)							\
     ? (0x64180000 + (VAL))	/* daddiu t8,zero,VAL sign extended */	\
     : (0x24180000 + (VAL))))	/* addiu t8,zero,VAL sign extended */
 
-#define MIPS_FUNCTION_STUB_SIZE(INFO) \
-  (elf_hash_table (INFO)->dynsymcount > 65536 ? 20 : 16)
-
-#define MIPS_FUNCTION_STUB_MAX_SIZE 20
+#define MIPS_FUNCTION_STUB_NORMAL_SIZE 16
+#define MIPS_FUNCTION_STUB_BIG_SIZE 20
 
 /* The name of the dynamic interpreter.  This is put in the .interp
    section.  */
@@ -6832,7 +6833,9 @@ _bfd_mips_elf_adjust_dynamic_symbol (str
   bfd *dynobj;
   struct mips_elf_link_hash_entry *hmips;
   asection *s;
+  struct mips_elf_link_hash_table *htab;
 
+  htab = mips_elf_hash_table (info);
   dynobj = elf_hash_table (info)->dynobj;
 
   /* Make sure we know what is going on here.  */
@@ -6885,7 +6888,7 @@ _bfd_mips_elf_adjust_dynamic_symbol (str
 	  h->plt.offset = s->size;
 
 	  /* Make room for this stub code.  */
-	  s->size += MIPS_FUNCTION_STUB_SIZE (info);
+	  s->size += htab->function_stub_size;
 
 	  /* The last half word of the stub will be filled with the index
 	     of this symbol in .dynsym section.  */
@@ -7073,6 +7076,32 @@ _bfd_mips_vxworks_adjust_dynamic_symbol 
   return TRUE;
 }
 \f
+/* Return the number of dynamic section symbols required by OUTPUT_BFD.
+   The number might be exact or a worst-case estimate, depending on how
+   much information is available to elf_backend_omit_section_dynsym at
+   the current linking stage.  */
+
+static bfd_size_type
+count_section_dynsyms (bfd *output_bfd, struct bfd_link_info *info)
+{
+  bfd_size_type count;
+
+  count = 0;
+  if (info->shared)
+    {
+      asection *p;
+      const struct elf_backend_data *bed;
+
+      bed = get_elf_backend_data (output_bfd);
+      for (p = output_bfd->sections; p ; p = p->next)
+	if ((p->flags & SEC_EXCLUDE) == 0
+	    && (p->flags & SEC_ALLOC) != 0
+	    && !(*bed->elf_backend_omit_section_dynsym) (output_bfd, info, p))
+	  ++count;
+    }
+  return count;
+}
+
 /* This function is called after all the input files have been read,
    and the input sections have been assigned to output sections.  We
    check for any mips16 stub sections that we can discard.  */
@@ -7089,6 +7118,7 @@ _bfd_mips_elf_always_size_sections (bfd 
   int i;
   bfd_size_type loadable_size = 0;
   bfd_size_type local_gotno;
+  bfd_size_type dynsymcount;
   bfd *sub;
   struct mips_elf_count_tls_arg count_tls_arg;
   struct mips_elf_link_hash_table *htab;
@@ -7147,10 +7177,22 @@ _bfd_mips_elf_always_size_sections (bfd 
        relocations, then GLOBAL_GOTSYM will be NULL.  */
     i = 0;
 
+  /* Get a worst-case estimate of the number of dynamic symbols needed.
+     At this point, dynsymcount does not account for section symbols
+     and count_section_dynsyms may overestimate the number that will
+     be needed.  */
+  dynsymcount = (elf_hash_table (info)->dynsymcount
+		 + count_section_dynsyms (output_bfd, info));
+
+  /* Determine the size of one stub entry.  */
+  htab->function_stub_size = (dynsymcount > 0x10000
+			      ? MIPS_FUNCTION_STUB_BIG_SIZE
+			      : MIPS_FUNCTION_STUB_NORMAL_SIZE);
+
   /* In the worst case, we'll get one stub per dynamic symbol, plus
      one to account for the dummy entry at the end required by IRIX
      rld.  */
-  loadable_size += MIPS_FUNCTION_STUB_SIZE (info) * (i + 1);
+  loadable_size += htab->function_stub_size * (i + 1);
 
   if (htab->is_vxworks)
     /* There's no need to allocate page entries for VxWorks; R_MIPS_GOT16
@@ -7367,14 +7409,14 @@ _bfd_mips_elf_size_dynamic_sections (bfd
       else if (strcmp (name, MIPS_ELF_STUB_SECTION_NAME (output_bfd)) == 0)
 	{
 	  /* IRIX rld assumes that the function stub isn't at the end
-	     of .text section. So put a dummy. XXX  */
-	  s->size += MIPS_FUNCTION_STUB_SIZE (info);
+	     of .text section.  So put a dummy.  XXX  */
+	  s->size += htab->function_stub_size;
 	}
       else if (! info->shared
 	       && ! mips_elf_hash_table (info)->use_rld_obj_head
 	       && strncmp (name, ".rld_map", 8) == 0)
 	{
-	  /* We add a room for __rld_map. It will be filled in by the
+	  /* We add a room for __rld_map.  It will be filled in by the
 	     rtld to contain a pointer to the _r_debug structure.  */
 	  s->size += 4;
 	}
@@ -8006,13 +8048,15 @@ _bfd_mips_elf_finish_dynamic_symbol (bfd
   struct mips_got_info *g, *gg;
   const char *name;
   int idx;
+  struct mips_elf_link_hash_table *htab;
 
+  htab = mips_elf_hash_table (info);
   dynobj = elf_hash_table (info)->dynobj;
 
   if (h->plt.offset != MINUS_ONE)
     {
       asection *s;
-      bfd_byte stub[MIPS_FUNCTION_STUB_MAX_SIZE];
+      bfd_byte stub[MIPS_FUNCTION_STUB_BIG_SIZE];
 
       /* This symbol has a stub.  Set it up.  */
 
@@ -8022,13 +8066,13 @@ _bfd_mips_elf_finish_dynamic_symbol (bfd
 				   MIPS_ELF_STUB_SECTION_NAME (dynobj));
       BFD_ASSERT (s != NULL);
 
-      BFD_ASSERT ((MIPS_FUNCTION_STUB_SIZE (info) == 20)
-                  || (h->dynindx <= 65536));
+      BFD_ASSERT ((htab->function_stub_size == MIPS_FUNCTION_STUB_BIG_SIZE)
+                  || (h->dynindx <= 0xffff));
 
       /* Values up to 2^31 - 1 are allowed.  Larger values would cause
-         sign extension at runtime in the stub, resulting in a
-         negative index value.  */
-      if (h->dynindx & 0x80000000)
+	 sign extension at runtime in the stub, resulting in a negative
+	 index value.  */
+      if (h->dynindx & ~0x7fffffff)
 	return FALSE;
 
       /* Fill the stub.  */
@@ -8037,9 +8081,9 @@ _bfd_mips_elf_finish_dynamic_symbol (bfd
       idx += 4;
       bfd_put_32 (output_bfd, STUB_MOVE (output_bfd), stub + idx);
       idx += 4;
-      if (MIPS_FUNCTION_STUB_SIZE (info) == 20)
+      if (htab->function_stub_size == MIPS_FUNCTION_STUB_BIG_SIZE)
         {
-          bfd_put_32 (output_bfd, STUB_LUI ((h->dynindx >> 16 ) & 0xffff),
+          bfd_put_32 (output_bfd, STUB_LUI ((h->dynindx >> 16) & 0x7fff),
                       stub + idx);
           idx += 4;
         }
@@ -8048,15 +8092,16 @@ _bfd_mips_elf_finish_dynamic_symbol (bfd
 
       /* If a large stub is not required and sign extension is not a
          problem, then use legacy code in the stub.  */
-      if ((MIPS_FUNCTION_STUB_SIZE (info) == 20) || (h->dynindx & 0xffff8000))
+      if (htab->function_stub_size == MIPS_FUNCTION_STUB_BIG_SIZE)
+	bfd_put_32 (output_bfd, STUB_ORI (h->dynindx & 0xffff), stub + idx);
+      else if (h->dynindx & ~0x7fff)
         bfd_put_32 (output_bfd, STUB_LI16U (h->dynindx & 0xffff), stub + idx);
       else
-        bfd_put_32 (output_bfd,
-                    STUB_LI16S (output_bfd, h->dynindx & 0xffff), stub + idx);
-        
+        bfd_put_32 (output_bfd, STUB_LI16S (output_bfd, h->dynindx),
+		    stub + idx);
+
       BFD_ASSERT (h->plt.offset <= s->size);
-      memcpy (s->contents + h->plt.offset,
-              stub, MIPS_FUNCTION_STUB_SIZE (info));
+      memcpy (s->contents + h->plt.offset, stub, htab->function_stub_size);
 
       /* Mark the symbol as undefined.  plt.offset != -1 occurs
 	 only for the referenced symbol.  */
@@ -8859,10 +8904,10 @@ _bfd_mips_elf_finish_dynamic_sections (b
 	      {
 		file_ptr dummy_offset;
 
-		BFD_ASSERT (s->size >= MIPS_FUNCTION_STUB_SIZE (info));
-		dummy_offset = s->size - MIPS_FUNCTION_STUB_SIZE (info);
+		BFD_ASSERT (s->size >= htab->function_stub_size);
+		dummy_offset = s->size - htab->function_stub_size;
 		memset (s->contents + dummy_offset, 0,
-			MIPS_FUNCTION_STUB_SIZE (info));
+			htab->function_stub_size);
 	      }
 	  }
       }
@@ -9974,6 +10019,7 @@ _bfd_mips_elf_link_hash_table_create (bf
   ret->splt = NULL;
   ret->plt_header_size = 0;
   ret->plt_entry_size = 0;
+  ret->function_stub_size = 0;
 
   return &ret->root.root;
 }
@@ -10049,18 +10095,7 @@ _bfd_mips_elf_final_link (bfd *abfd, str
 	 we count the sections after (possibly) removing the .options
 	 section above.  */
 
-      dynsecsymcount = 0;
-      if (info->shared)
-	{
-	  asection * p;
-
-	  for (p = abfd->sections; p ; p = p->next)
-	    if ((p->flags & SEC_EXCLUDE) == 0
-		&& (p->flags & SEC_ALLOC) != 0
-		&& !(*bed->elf_backend_omit_section_dynsym) (abfd, info, p))
-	      ++ dynsecsymcount;
-	}
-
+      dynsecsymcount = count_section_dynsyms (abfd, info);
       if (! mips_elf_sort_hash_table (info, dynsecsymcount + 1))
 	return FALSE;
 
Index: ld/testsuite/ld-mips-elf/mips-elf.exp
===================================================================
RCS file: /cvs/src/src/ld/testsuite/ld-mips-elf/mips-elf.exp,v
retrieving revision 1.35
diff -u -p -r1.35 mips-elf.exp
--- ld/testsuite/ld-mips-elf/mips-elf.exp	27 Mar 2006 11:30:54 -0000	1.35
+++ ld/testsuite/ld-mips-elf/mips-elf.exp	10 Jun 2006 10:16:53 -0000
@@ -137,6 +137,33 @@ if $has_newabi {
     run_dump_test "emit-relocs-1"
 }
 
+if {[istarget mips*-*-linux*]} {
+     # The number of symbols that are always included in the symbol table
+     # for these tests.  The 5 are:
+     #
+     #     the null symbol entry
+     #     the .MIPS.stubs section symbol
+     #     the .text section symbol
+     #     _gp
+     #     _GLOBAL_OFFSET_TABLE_
+     set base_syms 5
+     foreach dynsym { 7fff 8000 fff0 10000 2fe80 } {
+	 run_ld_link_tests \
+	     [list [list \
+			"Stub for dynsym 0x$dynsym" \
+			"-shared -melf32btsmip -T stub-dynsym-1.ld" \
+			[concat \
+			     "-EB -march=mips1 -32 -KPIC" \
+			     "--defsym base_syms=$base_syms" \
+			     "--defsym dynsym=0x$dynsym"] \
+			[list "stub-dynsym-1.s"] \
+			[list [list \
+				   "objdump" "-dz" \
+				   "stub-dynsym-1-$dynsym.d"]] \
+			"stub-dynsym-1-$dynsym"]]
+     }
+ }
+
 # For tests which may involve multiple files, use run_ld_link_tests.
 
 # List contains test-items with 3 items followed by 2 lists:
Index: ld/testsuite/ld-mips-elf/stub-dynsym-1-10000.d
===================================================================
RCS file: ld/testsuite/ld-mips-elf/stub-dynsym-1-10000.d
diff -N ld/testsuite/ld-mips-elf/stub-dynsym-1-10000.d
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-mips-elf/stub-dynsym-1-10000.d	10 Jun 2006 10:16:53 -0000
@@ -0,0 +1,18 @@
+
+.*:     file format elf32-tradbigmips
+
+Disassembly of section \.MIPS\.stubs:
+
+.* <\.MIPS.stubs>:
+.*:	8f998010 	lw	t9,-32752\(gp\)
+.*:	03e07821 	move	t7,ra
+.*:	3c180001 	lui	t8,0x1
+.*:	0320f809 	jalr	t9
+.*:	37180000 	ori	t8,t8,0x0
+.*:	00000000 	nop
+.*:	00000000 	nop
+.*:	00000000 	nop
+.*:	00000000 	nop
+.*:	00000000 	nop
+Disassembly of section .text:
+#pass
Index: ld/testsuite/ld-mips-elf/stub-dynsym-1-2fe80.d
===================================================================
RCS file: ld/testsuite/ld-mips-elf/stub-dynsym-1-2fe80.d
diff -N ld/testsuite/ld-mips-elf/stub-dynsym-1-2fe80.d
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-mips-elf/stub-dynsym-1-2fe80.d	10 Jun 2006 10:16:53 -0000
@@ -0,0 +1,18 @@
+
+.*:     file format elf32-tradbigmips
+
+Disassembly of section \.MIPS\.stubs:
+
+.* <\.MIPS.stubs>:
+.*:	8f998010 	lw	t9,-32752\(gp\)
+.*:	03e07821 	move	t7,ra
+.*:	3c180002 	lui	t8,0x2
+.*:	0320f809 	jalr	t9
+.*:	3718fe80 	ori	t8,t8,0xfe80
+.*:	00000000 	nop
+.*:	00000000 	nop
+.*:	00000000 	nop
+.*:	00000000 	nop
+.*:	00000000 	nop
+Disassembly of section .text:
+#pass
Index: ld/testsuite/ld-mips-elf/stub-dynsym-1-7fff.d
===================================================================
RCS file: ld/testsuite/ld-mips-elf/stub-dynsym-1-7fff.d
diff -N ld/testsuite/ld-mips-elf/stub-dynsym-1-7fff.d
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-mips-elf/stub-dynsym-1-7fff.d	10 Jun 2006 10:16:53 -0000
@@ -0,0 +1,16 @@
+
+.*:     file format elf32-tradbigmips
+
+Disassembly of section \.MIPS\.stubs:
+
+.* <\.MIPS.stubs>:
+.*:	8f998010 	lw	t9,-32752\(gp\)
+.*:	03e07821 	move	t7,ra
+.*:	0320f809 	jalr	t9
+.*:	24187fff 	li	t8,32767
+.*:	00000000 	nop
+.*:	00000000 	nop
+.*:	00000000 	nop
+.*:	00000000 	nop
+Disassembly of section .text:
+#pass
Index: ld/testsuite/ld-mips-elf/stub-dynsym-1-8000.d
===================================================================
RCS file: ld/testsuite/ld-mips-elf/stub-dynsym-1-8000.d
diff -N ld/testsuite/ld-mips-elf/stub-dynsym-1-8000.d
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-mips-elf/stub-dynsym-1-8000.d	10 Jun 2006 10:16:53 -0000
@@ -0,0 +1,16 @@
+
+.*:     file format elf32-tradbigmips
+
+Disassembly of section \.MIPS\.stubs:
+
+.* <\.MIPS.stubs>:
+.*:	8f998010 	lw	t9,-32752\(gp\)
+.*:	03e07821 	move	t7,ra
+.*:	0320f809 	jalr	t9
+.*:	34188000 	li	t8,0x8000
+.*:	00000000 	nop
+.*:	00000000 	nop
+.*:	00000000 	nop
+.*:	00000000 	nop
+Disassembly of section .text:
+#pass
Index: ld/testsuite/ld-mips-elf/stub-dynsym-1-fff0.d
===================================================================
RCS file: ld/testsuite/ld-mips-elf/stub-dynsym-1-fff0.d
diff -N ld/testsuite/ld-mips-elf/stub-dynsym-1-fff0.d
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-mips-elf/stub-dynsym-1-fff0.d	10 Jun 2006 10:16:53 -0000
@@ -0,0 +1,16 @@
+
+.*:     file format elf32-tradbigmips
+
+Disassembly of section \.MIPS\.stubs:
+
+.* <\.MIPS.stubs>:
+.*:	8f998010 	lw	t9,-32752\(gp\)
+.*:	03e07821 	move	t7,ra
+.*:	0320f809 	jalr	t9
+.*:	3418fff0 	li	t8,0xfff0
+.*:	00000000 	nop
+.*:	00000000 	nop
+.*:	00000000 	nop
+.*:	00000000 	nop
+Disassembly of section .text:
+#pass
Index: ld/testsuite/ld-mips-elf/stub-dynsym-1.ld
===================================================================
RCS file: ld/testsuite/ld-mips-elf/stub-dynsym-1.ld
diff -N ld/testsuite/ld-mips-elf/stub-dynsym-1.ld
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-mips-elf/stub-dynsym-1.ld	10 Jun 2006 10:16:53 -0000
@@ -0,0 +1,17 @@
+SECTIONS
+{
+  . = 0x80000;
+  .interp : { *(.interp) }
+  .hash : { *(.hash) }
+  .dynsym : { *(.dynsym) }
+  .dynstr : { *(.dynstr) }
+  .rel.dyn : { *(.rel.dyn) }
+  .MIPS.stubs : { *(.MIPS.stubs) }
+  .text : { *(.text) }
+
+  . = ALIGN (0x10000);
+  _gp = . + 0x7ff0;
+  .got : { *(.got) }
+
+  /DISCARD/ : { *(.reginfo) }
+}
Index: ld/testsuite/ld-mips-elf/stub-dynsym-1.s
===================================================================
RCS file: ld/testsuite/ld-mips-elf/stub-dynsym-1.s
diff -N ld/testsuite/ld-mips-elf/stub-dynsym-1.s
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-mips-elf/stub-dynsym-1.s	10 Jun 2006 10:16:53 -0000
@@ -0,0 +1,10 @@
+	.macro	decl
+	.global	exported\@
+	.equ	exported\@,\@
+	.endm
+
+	.rept	dynsym - base_syms
+	decl
+	.endr
+
+	lw	$25,%call16(foo)($gp)

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-10 16:04       ` Richard Sandiford
@ 2006-06-11  0:07         ` Eric Christopher
  2006-06-11  0:13         ` Thiemo Seufer
  1 sibling, 0 replies; 28+ messages in thread
From: Eric Christopher @ 2006-06-11  0:07 UTC (permalink / raw)
  To: Richard Sandiford; +Cc: Thiemo Seufer, David Daney, binutils, Daniel Jacobowitz

>
> Tested on mipsel-linux-gnu, mips64-linux-gnu and mipsisa64-elf.
> OK to install?
>
> bfd/
> 	* elfxx-mips.c (mips_elf_link_hash_table): Add function_stub_size.
> 	(STUB_ORI): New macro.
> 	(STUB_LI16U): Fix formatting.
> 	(MIPS_FUNCTION_STUB_SIZE): Delete.
> 	(MIPS_FUNCTION_STUB_MAX_SIZE): Likewise.
> 	(MIPS_FUNCTION_STUB_NORMAL_SIZE): New macro.
> 	(MIPS_FUNCTION_STUB_BIG_SIZE): Likewise.
> 	(_bfd_mips_elf_adjust_dynamic_symbol): Use htab->function_stub_size
> 	instead of MIPS_FUNCTION_STUB_SIZE.
> 	(count_section_dynsyms): New function, split out from
> 	_bfd_mips_elf_final_link.
> 	(_bfd_mips_elf_always_size_sections): Get a worst-case estimate
> 	of the number of dynamic symbols needed and use it to set up
> 	function_stub_size.  Use function_stub_size rather than
> 	MIPS_FUNCTION_STUB_SIZE to determine the size of the stub section.
> 	Use 16-byte stubs for 0x10000 dynamic symbols.
> 	(_bfd_mips_elf_size_dynamic_sections): Use htab->function_stub_size
> 	instead of MIPS_FUNCTION_STUB_SIZE.  Fix formatting.
> 	(_bfd_mips_elf_finish_dynamic_symbol): Likewise.  Change the
> 	size of the stub buffer from MIPS_FUNCTION_STUB_MAX_SIZE to
> 	MIPS_FUNCTION_STUB_BIG_SIZE.  Tweak the check for unhandled  
> dynindxes.
> 	Use MIPS_FUNCTION_STUB_BIG_SIZE rather than a hard-coded 20.
> 	Use STUB_ORI rather than STUB_LI16U for big stubs.
> 	(_bfd_mips_elf_link_hash_table_create): Initialize  
> function_stub_size.
> 	(_bfd_mips_elf_final_link): Use count_section_dynsyms.
>
> ld/testsuite/
> 	* ld-mips-elf/stub-dynsym-1.s,
> 	* ld-mips-elf/stub-dynsym-1.ld,
> 	* ld-mips-elf/stub-dynsym-1-7fff.d,
> 	* ld-mips-elf/stub-dynsym-1-8000.d,
> 	* ld-mips-elf/stub-dynsym-1-fff0.d,
> 	* ld-mips-elf/stub-dynsym-1-10000.d,
> 	* ld-mips-elf/stub-dynsym-1-2fe80.d: New test.
> 	* ld-mips-elf/mips-elf.exp: Run it.

OK.

-eric

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-10 16:04       ` Richard Sandiford
  2006-06-11  0:07         ` Eric Christopher
@ 2006-06-11  0:13         ` Thiemo Seufer
  2006-06-11  0:26           ` Daniel Jacobowitz
                             ` (2 more replies)
  1 sibling, 3 replies; 28+ messages in thread
From: Thiemo Seufer @ 2006-06-11  0:13 UTC (permalink / raw)
  To: David Daney, binutils, Daniel Jacobowitz, richard

Richard Sandiford wrote:
[snip]
> I've attached two patches.  The first is relative to Thiemo's patch
> from yesterday; the second is a complete patch against mainline.
> 
> Tested on mipsel-linux-gnu, mips64-linux-gnu and mipsisa64-elf.
> OK to install?
> 
> bfd/
> 	* elfxx-mips.c (mips_elf_link_hash_table): Add function_stub_size.
> 	(STUB_ORI): New macro.
> 	(STUB_LI16U): Fix formatting.
> 	(MIPS_FUNCTION_STUB_SIZE): Delete.
> 	(MIPS_FUNCTION_STUB_MAX_SIZE): Likewise.
> 	(MIPS_FUNCTION_STUB_NORMAL_SIZE): New macro.
> 	(MIPS_FUNCTION_STUB_BIG_SIZE): Likewise.
> 	(_bfd_mips_elf_adjust_dynamic_symbol): Use htab->function_stub_size
> 	instead of MIPS_FUNCTION_STUB_SIZE.
> 	(count_section_dynsyms): New function, split out from
> 	_bfd_mips_elf_final_link.
> 	(_bfd_mips_elf_always_size_sections): Get a worst-case estimate
> 	of the number of dynamic symbols needed and use it to set up
> 	function_stub_size.  Use function_stub_size rather than
> 	MIPS_FUNCTION_STUB_SIZE to determine the size of the stub section.
> 	Use 16-byte stubs for 0x10000 dynamic symbols.
> 	(_bfd_mips_elf_size_dynamic_sections): Use htab->function_stub_size
> 	instead of MIPS_FUNCTION_STUB_SIZE.  Fix formatting.
> 	(_bfd_mips_elf_finish_dynamic_symbol): Likewise.  Change the
> 	size of the stub buffer from MIPS_FUNCTION_STUB_MAX_SIZE to
> 	MIPS_FUNCTION_STUB_BIG_SIZE.  Tweak the check for unhandled dynindxes.
> 	Use MIPS_FUNCTION_STUB_BIG_SIZE rather than a hard-coded 20.
> 	Use STUB_ORI rather than STUB_LI16U for big stubs.
> 	(_bfd_mips_elf_link_hash_table_create): Initialize function_stub_size.
> 	(_bfd_mips_elf_final_link): Use count_section_dynsyms.
> 
> ld/testsuite/
> 	* ld-mips-elf/stub-dynsym-1.s,
> 	* ld-mips-elf/stub-dynsym-1.ld,
> 	* ld-mips-elf/stub-dynsym-1-7fff.d,
> 	* ld-mips-elf/stub-dynsym-1-8000.d,
> 	* ld-mips-elf/stub-dynsym-1-fff0.d,
> 	* ld-mips-elf/stub-dynsym-1-10000.d,
> 	* ld-mips-elf/stub-dynsym-1-2fe80.d: New test.
> 	* ld-mips-elf/mips-elf.exp: Run it.
> 

Ok.

I felt the urge to do some more cleanups (interdiff appended),
the only function change was to add the IRIX dummy entry to the
worst case calculation.

Don't let this one stop you to apply your patch. :-)


Thiemo


diff -u bfd/elfxx-mips.c bfd/elfxx-mips.c
--- bfd/elfxx-mips.c	10 Jun 2006 10:16:53 -0000
+++ bfd/elfxx-mips.c	10 Jun 2006 23:56:56 -0000
@@ -624,24 +624,7 @@
    offsets from $gp.  */
 #define MIPS_ELF_GOT_MAX_SIZE(INFO) (ELF_MIPS_GP_OFFSET (INFO) + 0x7fff)
 
-/* Instructions which appear in a stub.  */
-#define STUB_LW(abfd)							\
-  ((ABI_64_P (abfd)							\
-    ? 0xdf998010				/* ld t9,0x8010(gp) */	\
-    : 0x8f998010))              		/* lw t9,0x8010(gp) */
-#define STUB_MOVE(abfd)							\
-   ((ABI_64_P (abfd)							\
-     ? 0x03e0782d				/* daddu t7,ra */	\
-     : 0x03e07821))				/* addu t7,ra */
-#define STUB_LUI(VAL) (0x3c180000 + (VAL))	/* lui t8,VAL */
-#define STUB_JALR 0x0320f809			/* jalr t9,ra */
-#define STUB_ORI(VAL) (0x37180000 + (VAL))	/* ori t8,t8,VAL */
-#define STUB_LI16U(VAL) (0x34180000 + (VAL))	/* ori t8,zero,VAL unsigned */
-#define STUB_LI16S(abfd, VAL)						\
-   ((ABI_64_P (abfd)							\
-    ? (0x64180000 + (VAL))	/* daddiu t8,zero,VAL sign extended */	\
-    : (0x24180000 + (VAL))))	/* addiu t8,zero,VAL sign extended */
-
+/* Function stub sizes as created by mips_elf_build_function_stub.  */
 #define MIPS_FUNCTION_STUB_NORMAL_SIZE 16
 #define MIPS_FUNCTION_STUB_BIG_SIZE 20
 
@@ -7180,9 +7163,10 @@
   /* Get a worst-case estimate of the number of dynamic symbols needed.
      At this point, dynsymcount does not account for section symbols
      and count_section_dynsyms may overestimate the number that will
-     be needed.  */
+     be needed.  Also account for the IRIX dummy entry.  */
   dynsymcount = (elf_hash_table (info)->dynsymcount
-		 + count_section_dynsyms (output_bfd, info));
+		 + count_section_dynsyms (output_bfd, info)
+		 + 1);
 
   /* Determine the size of one stub entry.  */
   htab->function_stub_size = (dynsymcount > 0x10000
@@ -8034,6 +8018,66 @@
 	}
 }
 
+static bfd_boolean
+mips_elf_build_function_stub (bfd *output_bfd, struct bfd_link_info *info,
+			      struct elf_link_hash_entry *h, asection *s)
+{
+  bfd_byte *stub;
+  struct mips_elf_link_hash_table *htab;
+
+  htab = mips_elf_hash_table (info);
+  stub = s->contents + h->plt.offset;
+
+  BFD_ASSERT ((htab->function_stub_size == MIPS_FUNCTION_STUB_BIG_SIZE)
+              || (h->dynindx <= 0xffff));
+
+  /* Values up to 2^31 - 1 are allowed.  Larger values would cause sign
+     extension at runtime in the stub, resulting in a negative index
+     value.  */
+  if (h->dynindx & ~0x7fffffff)
+	return FALSE;
+
+  /* Fill the stub.  */
+  bfd_put_32 (output_bfd, (ABI_64_P (output_bfd)
+			   ? 0xdf998010		/* ld t9, 0x8010(gp) */
+			   : 0x8f998010),	/* lw t9, 0x8010(gp) */
+	      stub);
+  bfd_put_32 (output_bfd, (ABI_64_P (output_bfd)
+			   ? 0x03e0782d		/* daddu t7, ra */
+			   : 0x03e07821),	/* addu t7, ra */
+	      stub += 4);
+  if (htab->function_stub_size == MIPS_FUNCTION_STUB_BIG_SIZE)
+    {
+      /* lui t8, %hi(DYNINDX) */
+      bfd_put_32 (output_bfd, 0x3c180000 | ((h->dynindx >> 16) & 0x7fff),
+		  stub += 4);
+    }
+  bfd_put_32 (output_bfd, 0x0320f809, stub += 4); /* jalr t9, ra */
+  /* If a large stub is not required and sign extension is not a problem,
+     then use legacy code in the stub.  */
+  if (htab->function_stub_size == MIPS_FUNCTION_STUB_BIG_SIZE)
+    {
+      /* ori t8, t8, %lo(DYNINDX) # unsigned */
+      bfd_put_32 (output_bfd, 0x37180000 | (h->dynindx & 0xffff), stub += 4);
+    }
+  else if (h->dynindx & ~0x7fff)
+    {
+      /* ori t8, zero, %lo(DYNINDX) # unsigned */
+      bfd_put_32 (output_bfd, 0x34180000 | (h->dynindx & 0xffff), stub += 4);
+    }
+  else
+    {
+      bfd_put_32 (output_bfd, (ABI_64_P (output_bfd)
+			       /* daddiu t8, zero, %lo(DYNINDX) # signed */
+			       ? (0x64180000 + h->dynindx)
+			       /* addiu t8, zero, %lo(DYNINDX) # signed */
+			       : (0x24180000 + h->dynindx)), stub += 4);
+    }
+
+  BFD_ASSERT (h->plt.offset <= s->size);
+  return TRUE;
+}
+
 /* Finish up dynamic symbol handling.  We set the contents of various
    dynamic sections here.  */
 
@@ -8047,7 +8091,6 @@
   asection *sgot;
   struct mips_got_info *g, *gg;
   const char *name;
-  int idx;
   struct mips_elf_link_hash_table *htab;
 
   htab = mips_elf_hash_table (info);
@@ -8056,7 +8099,6 @@
   if (h->plt.offset != MINUS_ONE)
     {
       asection *s;
-      bfd_byte stub[MIPS_FUNCTION_STUB_BIG_SIZE];
 
       /* This symbol has a stub.  Set it up.  */
 
@@ -8067,41 +8109,7 @@
       BFD_ASSERT (s != NULL);
 
-      BFD_ASSERT ((htab->function_stub_size == MIPS_FUNCTION_STUB_BIG_SIZE)
-                  || (h->dynindx <= 0xffff));
-
-      /* Values up to 2^31 - 1 are allowed.  Larger values would cause
-	 sign extension at runtime in the stub, resulting in a negative
-	 index value.  */
-      if (h->dynindx & ~0x7fffffff)
+      if (!mips_elf_build_function_stub (output_bfd, info, h, s))
 	return FALSE;
 
-      /* Fill the stub.  */
-      idx = 0;
-      bfd_put_32 (output_bfd, STUB_LW (output_bfd), stub + idx);
-      idx += 4;
-      bfd_put_32 (output_bfd, STUB_MOVE (output_bfd), stub + idx);
-      idx += 4;
-      if (htab->function_stub_size == MIPS_FUNCTION_STUB_BIG_SIZE)
-        {
-          bfd_put_32 (output_bfd, STUB_LUI ((h->dynindx >> 16) & 0x7fff),
-                      stub + idx);
-          idx += 4;
-        }
-      bfd_put_32 (output_bfd, STUB_JALR, stub + idx);
-      idx += 4;
-
-      /* If a large stub is not required and sign extension is not a
-         problem, then use legacy code in the stub.  */
-      if (htab->function_stub_size == MIPS_FUNCTION_STUB_BIG_SIZE)
-	bfd_put_32 (output_bfd, STUB_ORI (h->dynindx & 0xffff), stub + idx);
-      else if (h->dynindx & ~0x7fff)
-        bfd_put_32 (output_bfd, STUB_LI16U (h->dynindx & 0xffff), stub + idx);
-      else
-        bfd_put_32 (output_bfd, STUB_LI16S (output_bfd, h->dynindx),
-		    stub + idx);
-
-      BFD_ASSERT (h->plt.offset <= s->size);
-      memcpy (s->contents + h->plt.offset, stub, htab->function_stub_size);
-
       /* Mark the symbol as undefined.  plt.offset != -1 occurs
 	 only for the referenced symbol.  */
@@ -8114,8 +8122,7 @@
 		       + h->plt.offset);
     }
 
-  BFD_ASSERT (h->dynindx != -1
-	      || h->forced_local);
+  BFD_ASSERT (h->dynindx != -1 || h->forced_local);
 
   sgot = mips_elf_got_section (dynobj, FALSE);
   BFD_ASSERT (sgot != NULL);

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-11  0:13         ` Thiemo Seufer
@ 2006-06-11  0:26           ` Daniel Jacobowitz
  2006-06-11  0:42           ` Eric Christopher
  2006-06-11 13:27           ` Richard Sandiford
  2 siblings, 0 replies; 28+ messages in thread
From: Daniel Jacobowitz @ 2006-06-11  0:26 UTC (permalink / raw)
  To: Thiemo Seufer; +Cc: David Daney, binutils, richard

On Sun, Jun 11, 2006 at 01:07:45AM +0100, Thiemo Seufer wrote:
> Ok.
> 
> I felt the urge to do some more cleanups (interdiff appended),
> the only function change was to add the IRIX dummy entry to the
> worst case calculation.
> 
> Don't let this one stop you to apply your patch. :-)

When everyone's decided on this, and some patch or other has been
committed to HEAD, I would appreciate it if (A) it could be committed
to the branch too, and then (B) someone could let me know.

-- 
Daniel Jacobowitz
CodeSourcery

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-11  0:13         ` Thiemo Seufer
  2006-06-11  0:26           ` Daniel Jacobowitz
@ 2006-06-11  0:42           ` Eric Christopher
  2006-06-11  8:21             ` Thiemo Seufer
  2006-06-11 13:27           ` Richard Sandiford
  2 siblings, 1 reply; 28+ messages in thread
From: Eric Christopher @ 2006-06-11  0:42 UTC (permalink / raw)
  To: Thiemo Seufer; +Cc: David Daney, binutils, Daniel Jacobowitz, richard

I usually prefer this style:

> -#define STUB_LUI(VAL) (0x3c180000 + (VAL))	/* lui t8,VAL */
>


To this...

> +  /* Fill the stub.  */
> +  bfd_put_32 (output_bfd, (ABI_64_P (output_bfd)
> +			   ? 0xdf998010		/* ld t9, 0x8010(gp) */
> +			   : 0x8f998010),	/* lw t9, 0x8010(gp) */
> +	      stub);

-eric

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-11  0:42           ` Eric Christopher
@ 2006-06-11  8:21             ` Thiemo Seufer
  2006-06-11  8:41               ` Richard Sandiford
  0 siblings, 1 reply; 28+ messages in thread
From: Thiemo Seufer @ 2006-06-11  8:21 UTC (permalink / raw)
  To: Eric Christopher; +Cc: David Daney, binutils, Daniel Jacobowitz, richard

Eric Christopher wrote:
> I usually prefer this style:
> 
> >-#define STUB_LUI(VAL) (0x3c180000 + (VAL))	/* lui t8,VAL */

plus

>-          bfd_put_32 (output_bfd, STUB_LUI ((h->dynindx >> 16) & 0x7fff),
>-                      stub + idx);
>-          idx += 4;

or rather:

>-/* Instructions which appear in a stub.  */
>-#define STUB_LW(abfd)                                                  \
>-  ((ABI_64_P (abfd)                                                    \
>-    ? 0xdf998010                               /* ld t9,0x8010(gp) */  \
>-    : 0x8f998010))                             /* lw t9,0x8010(gp) */

plus

>-      idx = 0;
>-      bfd_put_32 (output_bfd, STUB_LW (output_bfd), stub + idx);

elsewhere ...

> To this...
> 
> >+  /* Fill the stub.  */
> >+  bfd_put_32 (output_bfd, (ABI_64_P (output_bfd)
> >+			   ? 0xdf998010		/* ld t9, 0x8010(gp) */
> >+			   : 0x8f998010),	/* lw t9, 0x8010(gp) */
> >+	      stub);

All those macros are only used once, so I prefer to have the
information in a single place in the file.


Thiemo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-11  8:21             ` Thiemo Seufer
@ 2006-06-11  8:41               ` Richard Sandiford
  2006-06-11 15:46                 ` David Daney
  0 siblings, 1 reply; 28+ messages in thread
From: Richard Sandiford @ 2006-06-11  8:41 UTC (permalink / raw)
  To: Thiemo Seufer; +Cc: Eric Christopher, David Daney, binutils, Daniel Jacobowitz

Thiemo Seufer <ths@networkno.de> writes:
> Eric Christopher wrote:
>> I usually prefer this style:
>> 
>> >-#define STUB_LUI(VAL) (0x3c180000 + (VAL))	/* lui t8,VAL */
>
> plus
>
>>-          bfd_put_32 (output_bfd, STUB_LUI ((h->dynindx >> 16) & 0x7fff),
>>-                      stub + idx);
>>-          idx += 4;
>
> or rather:
>
>>-/* Instructions which appear in a stub.  */
>>-#define STUB_LW(abfd)                                                  \
>>-  ((ABI_64_P (abfd)                                                    \
>>-    ? 0xdf998010                               /* ld t9,0x8010(gp) */  \
>>-    : 0x8f998010))                             /* lw t9,0x8010(gp) */
>
> plus
>
>>-      idx = 0;
>>-      bfd_put_32 (output_bfd, STUB_LW (output_bfd), stub + idx);
>
> elsewhere ...
>
>> To this...
>> 
>> >+  /* Fill the stub.  */
>> >+  bfd_put_32 (output_bfd, (ABI_64_P (output_bfd)
>> >+			   ? 0xdf998010		/* ld t9, 0x8010(gp) */
>> >+			   : 0x8f998010),	/* lw t9, 0x8010(gp) */
>> >+	      stub);
>
> All those macros are only used once, so I prefer to have the
> information in a single place in the file.

FWIW, I agree with Eric here.  Having macros for hard-coded constants
is good, even if they're only used once.  IOW I prefer the way it is now.
I don't feel strongly enough to argue about it further though. ;)

Dan, I've applied the patch I posted to HEAD and 2.17 branch.

Richard

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-11  0:13         ` Thiemo Seufer
  2006-06-11  0:26           ` Daniel Jacobowitz
  2006-06-11  0:42           ` Eric Christopher
@ 2006-06-11 13:27           ` Richard Sandiford
  2006-06-11 19:29             ` Thiemo Seufer
  2 siblings, 1 reply; 28+ messages in thread
From: Richard Sandiford @ 2006-06-11 13:27 UTC (permalink / raw)
  To: Thiemo Seufer; +Cc: David Daney, binutils, Daniel Jacobowitz

Thiemo Seufer <ths@networkno.de> writes:
> I felt the urge to do some more cleanups (interdiff appended),
> the only function change was to add the IRIX dummy entry to the
> worst case calculation.

Which IRIX dummy entry?  If you're referring to the dummy entry at
the beginning of the symbol table, that entry is a gABI feature and
is already accounted for.

Richard

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more  than 2^15 symbols.
  2006-06-11  8:41               ` Richard Sandiford
@ 2006-06-11 15:46                 ` David Daney
  0 siblings, 0 replies; 28+ messages in thread
From: David Daney @ 2006-06-11 15:46 UTC (permalink / raw)
  To: richard; +Cc: Thiemo Seufer, Eric Christopher, binutils, Daniel Jacobowitz

Richard Sandiford wrote:
> Dan, I've applied the patch I posted to HEAD and 2.17 branch.
>
>   
Richard,

Thanks for cleaning up the mess.

I think it was a case in point as to the value of test cases.

David Daney

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-11 13:27           ` Richard Sandiford
@ 2006-06-11 19:29             ` Thiemo Seufer
  2006-06-11 20:03               ` Richard Sandiford
  0 siblings, 1 reply; 28+ messages in thread
From: Thiemo Seufer @ 2006-06-11 19:29 UTC (permalink / raw)
  To: David Daney, binutils, Daniel Jacobowitz, richard

Richard Sandiford wrote:
> Thiemo Seufer <ths@networkno.de> writes:
> > I felt the urge to do some more cleanups (interdiff appended),
> > the only function change was to add the IRIX dummy entry to the
> > worst case calculation.
> 
> Which IRIX dummy entry?  If you're referring to the dummy entry at
> the beginning of the symbol table, that entry is a gABI feature and
> is already accounted for.

The one mentioned immediately below:

  /* In the worst case, we'll get one stub per dynamic symbol, plus
     to account for the dummy entry at the end required by IRIX
     rld.  */

and also

  /* IRIX rld assumes that the function stub isn't at the end
     of .text section.  So put a dummy.  XXX  */


Thiemo

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-11 19:29             ` Thiemo Seufer
@ 2006-06-11 20:03               ` Richard Sandiford
  2006-06-11 20:42                 ` Thiemo Seufer
  0 siblings, 1 reply; 28+ messages in thread
From: Richard Sandiford @ 2006-06-11 20:03 UTC (permalink / raw)
  To: Thiemo Seufer; +Cc: David Daney, binutils, Daniel Jacobowitz

Thiemo Seufer <ths@networkno.de> writes:
> Richard Sandiford wrote:
>> Thiemo Seufer <ths@networkno.de> writes:
>> > I felt the urge to do some more cleanups (interdiff appended),
>> > the only function change was to add the IRIX dummy entry to the
>> > worst case calculation.
>> 
>> Which IRIX dummy entry?  If you're referring to the dummy entry at
>> the beginning of the symbol table, that entry is a gABI feature and
>> is already accounted for.
>
> The one mentioned immediately below:
>
>   /* In the worst case, we'll get one stub per dynamic symbol, plus
>      to account for the dummy entry at the end required by IRIX
>      rld.  */
>
> and also
>
>   /* IRIX rld assumes that the function stub isn't at the end
>      of .text section.  So put a dummy.  XXX  */

I don't understand.  The code you changed in your patch is counting
_symbols_ not stubs:

  /* Get a worst-case estimate of the number of dynamic symbols needed.
     At this point, dynsymcount does not account for section symbols
     and count_section_dynsyms may overestimate the number that will
     be needed.  */
  dynsymcount = (elf_hash_table (info)->dynsymcount
		 + count_section_dynsyms (output_bfd, info));

That's the calculation you added 1 to.  The code that counts the number
of _stubs_ already takes the dummy stub into account.  You've quoted the
comment for that code yourself ;)

  /* In the worst case, we'll get one stub per dynamic symbol, plus
     one to account for the dummy entry at the end required by IRIX
     rld.  */
  loadable_size += htab->function_stub_size * (i + 1);

If we'd got that wrong, we'd have been missing the dummy stub before
the current patch series.  Note that here, "per dynaamic symbol" means
"per GOT-mapped dynamic symbol" (i.e. all those after DT_MIPS_GOTSYM).
"i" already holds that value, and its calculation is not affected
by the recent patches.

Richard

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols.
  2006-06-11 20:03               ` Richard Sandiford
@ 2006-06-11 20:42                 ` Thiemo Seufer
  0 siblings, 0 replies; 28+ messages in thread
From: Thiemo Seufer @ 2006-06-11 20:42 UTC (permalink / raw)
  To: David Daney, binutils, Daniel Jacobowitz, richard

Richard Sandiford wrote:
[snip]
> >   /* IRIX rld assumes that the function stub isn't at the end
> >      of .text section.  So put a dummy.  XXX  */
> 
> I don't understand.  The code you changed in your patch is counting
> _symbols_ not stubs:

Ah, ok. I guess I'm now just happy to see it work instead of tinkering
in that place without putting enough thought in. :-)


Thiemo

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2006-06-11 20:03 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-06-08 23:03 [PATCH] (version 2) Change MIPS linker stubs to allow for more than 2^15 symbols David Daney
2006-06-08 23:12 ` Thiemo Seufer
2006-06-08 23:19   ` David Daney
2006-06-09  1:16     ` Thiemo Seufer
2006-06-09  0:01   ` David Daney
2006-06-09  6:38     ` David Daney
2006-06-08 23:35 ` Daniel Jacobowitz
2006-06-09  7:59 ` Richard Sandiford
2006-06-09 16:39   ` Thiemo Seufer
2006-06-09 18:18     ` David Daney
2006-06-09 18:23       ` Thiemo Seufer
2006-06-09 19:40         ` David Daney
2006-06-09 18:28     ` Daniel Jacobowitz
2006-06-09 19:24       ` David Daney
2006-06-09 19:27         ` Paul Koning
2006-06-09 21:00     ` Richard Sandiford
2006-06-10 16:04       ` Richard Sandiford
2006-06-11  0:07         ` Eric Christopher
2006-06-11  0:13         ` Thiemo Seufer
2006-06-11  0:26           ` Daniel Jacobowitz
2006-06-11  0:42           ` Eric Christopher
2006-06-11  8:21             ` Thiemo Seufer
2006-06-11  8:41               ` Richard Sandiford
2006-06-11 15:46                 ` David Daney
2006-06-11 13:27           ` Richard Sandiford
2006-06-11 19:29             ` Thiemo Seufer
2006-06-11 20:03               ` Richard Sandiford
2006-06-11 20:42                 ` Thiemo Seufer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).