Patch for MIPS multi-got bug with forced-local symbols

Patch for MIPS multi-got bug with forced-local symbols

[Joseph S. Myers]

This patch fixes a MIPS multi-GOT bug (assertion failures followed by
other link errors) shown by the testcase added.

The symbols __init_array_start and __init_array_end are first
processed as undefined hidden references, then they are defined by a
PROVIDE_HIDDEN in the linker script.

Inconsistencies arise in how these symbols are counted for multi-GOT
allocation.  mips_elf_sort_hash_table starts with

  hsd.max_unref_got_dynindx =
  hsd.min_got_dynindx = elf_hash_table (info)->dynsymcount
    /* In the multi-got case, assigned_gotno of the master got_info
       indicate the number of entries that aren't referenced in the
       primary GOT, but that must have entries because there are
       dynamic relocations that reference it.  Since they aren't
       referenced, we move them to the end of the GOT, so that they
       don't prevent other entries that are referenced from getting
       too large offsets.  */
    - (g->next ? g->assigned_gotno : 0);

and assigned_gotno was calculated in mips_elf_multi_got,

      gg->assigned_gotno = gg->global_gotno - g->global_gotno;

but this is subtracting figures calculated in two different ways.
gg->global_gotno, the figure for the master GOT, is calculated in
_bfd_mips_elf_always_size_sections:

  if (g->global_gotsym != NULL)
    i = elf_hash_table (info)->dynsymcount - g->global_gotsym->dynindx;
...
  g->global_gotno = i;

(and includes __init_array_*: they were assigned dynindx values when
first processed because they were undefined at that point) while
g->global_gotno, the figure for the primary GOT, is calculated more
directly by a series of increments in mips_elf_make_got_per_bfd, where
the conditionals

  else if (entry->symndx >= 0 || entry->d.h->forced_local)
    ++g->local_gotno;
  else
    ++g->global_gotno;

ensure that the forced-local __init_array_* are not counted as global.
Now the code in mips_elf_multi_got

      set_got_offset_arg.value = 2;
...
  /* Reorder dynamic symbols as described above (which behavior
     depends on the setting of VALUE).  */
  set_got_offset_arg.g = NULL;
  htab_traverse (gg->got_entries, mips_elf_set_global_got_offset,
                 &set_got_offset_arg);
  set_got_offset_arg.value = 1;
  htab_traverse (g->got_entries, mips_elf_set_global_got_offset,
                 &set_got_offset_arg);
  if (! mips_elf_sort_hash_table (info, 1))
    return FALSE;

sets the offsets for __init_array_* to 2 and then to 1, so
mips_elf_sort_hash_table_f treats them as global GOT entries,
explicitly referenced, which would only be correct for entries *not*
included in that assigned_gotno figure.  (These offsets were
previously set to 1 in mips_elf_record_global_got_symbol so stopping
mips_elf_set_global_got_offset from touching them doesn't make any
difference.)

The patch deals with the problem by counting those forced-local
symbols that have been assigned dynindx values and adjusting the
calculations accordingly.  Tested with cross to mips-linux-gnu,
binutils testsuites; also made sure it will build a working glibc for
O32, N32 and N64.  OK to commit?

bfd:
2007-09-19  Joseph Myers  <joseph@codesourcery.com>

	* elfxx-mips.c (struct mips_got_info): Add forced_local_count.
	(struct mips_elf_hash_sort_data): Add forced_local and
	prev_forced_local.
	(mips_elf_sort_hash_table): Subtract g->forced_local_count in
	computing hsd.min_got_dynindx.  Initialize hsd.forced_local and
	hsd.prev_forced_local.  Set g->forced_local_count after sorting.
	(mips_elf_sort_hash_table_f): Count forced-local symbols.  Handle
	them as unreferenced where allowed for in calculation of
	min_got_dynindx.
	(mips_elf_make_got_per_bfd, mips_elf_multi_got,
	mips_elf_create_got_section): Initialize forced_local_count.
	(_bfd_mips_elf_always_size_sections): Subtract forced_local_count
	in calculating global_gotno.
	(_bfd_mips_elf_final_link): Subtract forced_local_count in
	assertion.

ld/testsuite:
2007-09-19  Joseph Myers  <joseph@codesourcery.com>

	* ld-mips-elf/multi-got-hidden.d, ld-mips-elf/multi-got-hidden.s:
	New.
	* ld-mips-elf/mips-elf.exp: Run multi-got-hidden test.

Index: bfd/elfxx-mips.c
===================================================================
RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
retrieving revision 1.213
diff -u -r1.213 elfxx-mips.c
--- bfd/elfxx-mips.c	13 Aug 2007 21:16:38 -0000	1.213
+++ bfd/elfxx-mips.c	19 Sep 2007 11:53:47 -0000
@@ -143,6 +143,9 @@
      because a single-GOT link may have multiple hash table entries
      for the LDM.  It does not get initialized in multi-GOT mode.  */
   bfd_vma tls_ldm_offset;
+  /* The number of forced-local entries being counted as global found
+     during sorting.  */
+  unsigned int forced_local_count;
 };
 
 /* Map an input bfd to a got in a multi-got link.  */
@@ -234,6 +237,10 @@
   /* The greatest dynamic symbol table index not corresponding to a
      symbol without a GOT entry.  */
   long max_non_got_dynindx;
+  /* The number of forced-local entries being counted as global.  */
+  long forced_local;
+  /* The number of such entries found previously.  */
+  long prev_forced_local;
 };
 
 /* The MIPS ELF linker needs additional information for each symbol in
@@ -2757,8 +2764,10 @@
        referenced, we move them to the end of the GOT, so that they
        don't prevent other entries that are referenced from getting
        too large offsets.  */
-    - (g->next ? g->assigned_gotno : 0);
+    - (g->next ? g->assigned_gotno : 0) - g->forced_local_count;
   hsd.max_non_got_dynindx = max_local;
+  hsd.forced_local = 0;
+  hsd.prev_forced_local = g->forced_local_count;
   mips_elf_link_hash_traverse (((struct mips_elf_link_hash_table *)
 				elf_hash_table (info)),
 			       mips_elf_sort_hash_table_f,
@@ -2774,6 +2783,8 @@
      table index in the GOT.  */
   g->global_gotsym = hsd.low;
 
+  g->forced_local_count = hsd.forced_local;
+
   return TRUE;
 }
 
@@ -2794,6 +2805,9 @@
   if (h->root.dynindx == -1)
     return TRUE;
 
+  if (h->forced_local)
+    hsd->forced_local++;
+
   /* Global symbols that need GOT entries that are not explicitly
      referenced are marked with got offset 2.  Those that are
      referenced get a 1, and those that don't need GOT entries get
@@ -2812,8 +2826,13 @@
     {
       BFD_ASSERT (h->tls_type == GOT_NORMAL);
 
-      h->root.dynindx = --hsd->min_got_dynindx;
-      hsd->low = (struct elf_link_hash_entry *) h;
+      if (h->forced_local && hsd->forced_local <= hsd->prev_forced_local)
+	h->root.dynindx = hsd->max_unref_got_dynindx++;
+      else
+	{
+	  h->root.dynindx = --hsd->min_got_dynindx;
+	  hsd->low = (struct elf_link_hash_entry *) h;
+	}
     }
 
   return TRUE;
@@ -3044,6 +3063,7 @@
       g->tls_gotno = 0;
       g->tls_assigned_gotno = 0;
       g->tls_ldm_offset = MINUS_ONE;
+      g->forced_local_count = 0;
       g->got_entries = htab_try_create (1, mips_elf_multi_got_entry_hash,
 					mips_elf_multi_got_entry_eq, NULL);
       if (g->got_entries == NULL)
@@ -3451,6 +3471,7 @@
       g->next->assigned_gotno = 0;
       g->next->tls_assigned_gotno = 0;
       g->next->tls_ldm_offset = MINUS_ONE;
+      g->next->forced_local_count = 0;
       g->next->got_entries = htab_try_create (1, mips_elf_multi_got_entry_hash,
 					      mips_elf_multi_got_entry_eq,
 					      NULL);
@@ -3816,6 +3837,7 @@
   g->bfd2got = NULL;
   g->next = NULL;
   g->tls_ldm_offset = MINUS_ONE;
+  g->forced_local_count = 0;
   g->got_entries = htab_try_create (1, mips_elf_got_entry_hash,
 				    mips_elf_got_entry_eq, NULL);
   if (g->got_entries == NULL)
@@ -7252,7 +7274,8 @@
     return FALSE;
 
   if (g->global_gotsym != NULL)
-    i = elf_hash_table (info)->dynsymcount - g->global_gotsym->dynindx;
+    i = elf_hash_table (info)->dynsymcount - g->global_gotsym->dynindx
+	- g->forced_local_count;
   else
     /* If there are no global symbols, or none requiring
        relocations, then GLOBAL_GOTSYM will be NULL.  */
@@ -10306,7 +10329,8 @@
 
       if (g->global_gotsym != NULL)
 	BFD_ASSERT ((elf_hash_table (info)->dynsymcount
-		     - g->global_gotsym->dynindx)
+		     - g->global_gotsym->dynindx
+		     - g->forced_local_count)
 		    <= g->global_gotno);
     }
 
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.47
diff -u -r1.47 mips-elf.exp
--- ld/testsuite/ld-mips-elf/mips-elf.exp	13 Aug 2007 21:16:39 -0000	1.47
+++ ld/testsuite/ld-mips-elf/mips-elf.exp	19 Sep 2007 11:53:48 -0000
@@ -68,6 +68,7 @@
 if { $linux_gnu } {
     run_dump_test "multi-got-1"
     run_dump_test "multi-got-no-shared"
+    run_dump_test "multi-got-hidden"
 }
 
 if $has_newabi {
Index: ld/testsuite/ld-mips-elf/multi-got-hidden.d
===================================================================
RCS file: ld/testsuite/ld-mips-elf/multi-got-hidden.d
diff -N ld/testsuite/ld-mips-elf/multi-got-hidden.d
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-mips-elf/multi-got-hidden.d	19 Sep 2007 11:53:48 -0000
@@ -0,0 +1,8 @@
+#name: MIPS multi-got-hidden
+#as: -EB -32 -KPIC
+#source: multi-got-1-1.s
+#source: multi-got-1-2.s
+#source: multi-got-hidden.s
+#ld: -melf32btsmip -e 0
+#objdump: -dr
+#pass
Index: ld/testsuite/ld-mips-elf/multi-got-hidden.s
===================================================================
RCS file: ld/testsuite/ld-mips-elf/multi-got-hidden.s
diff -N ld/testsuite/ld-mips-elf/multi-got-hidden.s
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-mips-elf/multi-got-hidden.s	19 Sep 2007 11:53:48 -0000
@@ -0,0 +1,5 @@
+.hidden __init_array_end
+.hidden __init_array_start
+sym_3_1:
+la $2, __init_array_start
+la $2, __init_array_end

-- 
Joseph S. Myers
joseph@codesourcery.com

Ping Re: Patch for MIPS multi-got bug with forced-local symbols

[Joseph S. Myers]

Ping.  This patch 
<http://sourceware.org/ml/binutils/2007-09/msg00266.html> is pending 
review.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Ping Re: Patch for MIPS multi-got bug with forced-local symbols

[Joseph S. Myers]

Here is a revised version of this patch 
<http://sourceware.org/ml/binutils/2007-09/msg00266.html>.  I found a new 
related case of failure (with both unpatched binutils and binutils with my 
previous patch applied), as shown by the second new test added, and fixed 
by the

+      && !entry->d.h->forced_local

added to mips_elf_set_global_got_offset.

Tested with cross to mips-linux-gnu.  OK to commit?

bfd:
2007-09-28  Joseph Myers  <joseph@codesourcery.com>

	* elfxx-mips.c (struct mips_got_info): Add forced_local_count.
	(struct mips_elf_hash_sort_data): Add forced_local and
	prev_forced_local.
	(mips_elf_sort_hash_table): Subtract g->forced_local_count in
	computing hsd.min_got_dynindx.  Initialize hsd.forced_local and
	hsd.prev_forced_local.  Set g->forced_local_count after sorting.
	(mips_elf_sort_hash_table_f): Count forced-local symbols.  Handle
	them as unreferenced where allowed for in calculation of
	min_got_dynindx.
	(mips_elf_make_got_per_bfd, mips_elf_multi_got,
	mips_elf_create_got_section): Initialize forced_local_count.
	(_bfd_mips_elf_always_size_sections): Subtract forced_local_count
	in calculating global_gotno.
	(_bfd_mips_elf_final_link): Subtract forced_local_count in
	assertion.
	(mips_elf_set_global_got_offset): Check for forced-local symbols
	before assigning global GOT offsets.

ld/testsuite:
2007-09-28  Joseph Myers  <joseph@codesourcery.com>

	* ld-mips-elf/multi-got-hidden-1.d,
	ld-mips-elf/multi-got-hidden-1.s,
	ld-mips-elf/multi-got-hidden-2.d,
	ld-mips-elf/multi-got-hidden-2.s: New.
	* ld-mips-elf/mips-elf.exp: Run multi-got-hidden tests.

(To keep down the size of this patch, multi-got-hidden-2.s is omitted;
it's the result of "cat multi-got-1-2.s multi-got-hidden-1.s".)

Index: bfd/elfxx-mips.c
===================================================================
RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
retrieving revision 1.215
diff -u -r1.215 elfxx-mips.c
--- bfd/elfxx-mips.c	26 Sep 2007 13:45:32 -0000	1.215
+++ bfd/elfxx-mips.c	28 Sep 2007 22:40:43 -0000
@@ -143,6 +143,9 @@
      because a single-GOT link may have multiple hash table entries
      for the LDM.  It does not get initialized in multi-GOT mode.  */
   bfd_vma tls_ldm_offset;
+  /* The number of forced-local entries being counted as global found
+     during sorting.  */
+  unsigned int forced_local_count;
 };
 
 /* Map an input bfd to a got in a multi-got link.  */
@@ -234,6 +237,10 @@
   /* The greatest dynamic symbol table index not corresponding to a
      symbol without a GOT entry.  */
   long max_non_got_dynindx;
+  /* The number of forced-local entries being counted as global.  */
+  long forced_local;
+  /* The number of such entries found previously.  */
+  long prev_forced_local;
 };
 
 /* The MIPS ELF linker needs additional information for each symbol in
@@ -2757,8 +2764,10 @@
        referenced, we move them to the end of the GOT, so that they
        don't prevent other entries that are referenced from getting
        too large offsets.  */
-    - (g->next ? g->assigned_gotno : 0);
+    - (g->next ? g->assigned_gotno : 0) - g->forced_local_count;
   hsd.max_non_got_dynindx = max_local;
+  hsd.forced_local = 0;
+  hsd.prev_forced_local = g->forced_local_count;
   mips_elf_link_hash_traverse (((struct mips_elf_link_hash_table *)
 				elf_hash_table (info)),
 			       mips_elf_sort_hash_table_f,
@@ -2774,6 +2783,8 @@
      table index in the GOT.  */
   g->global_gotsym = hsd.low;
 
+  g->forced_local_count = hsd.forced_local;
+
   return TRUE;
 }
 
@@ -2794,6 +2805,9 @@
   if (h->root.dynindx == -1)
     return TRUE;
 
+  if (h->forced_local)
+    hsd->forced_local++;
+
   /* Global symbols that need GOT entries that are not explicitly
      referenced are marked with got offset 2.  Those that are
      referenced get a 1, and those that don't need GOT entries get
@@ -2812,8 +2826,13 @@
     {
       BFD_ASSERT (h->tls_type == GOT_NORMAL);
 
-      h->root.dynindx = --hsd->min_got_dynindx;
-      hsd->low = (struct elf_link_hash_entry *) h;
+      if (h->forced_local && hsd->forced_local <= hsd->prev_forced_local)
+	h->root.dynindx = hsd->max_unref_got_dynindx++;
+      else
+	{
+	  h->root.dynindx = --hsd->min_got_dynindx;
+	  hsd->low = (struct elf_link_hash_entry *) h;
+	}
     }
 
   return TRUE;
@@ -3044,6 +3063,7 @@
       g->tls_gotno = 0;
       g->tls_assigned_gotno = 0;
       g->tls_ldm_offset = MINUS_ONE;
+      g->forced_local_count = 0;
       g->got_entries = htab_try_create (1, mips_elf_multi_got_entry_hash,
 					mips_elf_multi_got_entry_eq, NULL);
       if (g->got_entries == NULL)
@@ -3269,6 +3289,7 @@
 
   if (entry->abfd != NULL && entry->symndx == -1
       && entry->d.h->root.dynindx != -1
+      && !entry->d.h->forced_local
       && entry->d.h->tls_type == GOT_NORMAL)
     {
       if (g)
@@ -3451,6 +3472,7 @@
       g->next->assigned_gotno = 0;
       g->next->tls_assigned_gotno = 0;
       g->next->tls_ldm_offset = MINUS_ONE;
+      g->next->forced_local_count = 0;
       g->next->got_entries = htab_try_create (1, mips_elf_multi_got_entry_hash,
 					      mips_elf_multi_got_entry_eq,
 					      NULL);
@@ -3816,6 +3838,7 @@
   g->bfd2got = NULL;
   g->next = NULL;
   g->tls_ldm_offset = MINUS_ONE;
+  g->forced_local_count = 0;
   g->got_entries = htab_try_create (1, mips_elf_got_entry_hash,
 				    mips_elf_got_entry_eq, NULL);
   if (g->got_entries == NULL)
@@ -7254,7 +7277,8 @@
     return FALSE;
 
   if (g->global_gotsym != NULL)
-    i = elf_hash_table (info)->dynsymcount - g->global_gotsym->dynindx;
+    i = elf_hash_table (info)->dynsymcount - g->global_gotsym->dynindx
+	- g->forced_local_count;
   else
     /* If there are no global symbols, or none requiring
        relocations, then GLOBAL_GOTSYM will be NULL.  */
@@ -10309,7 +10333,8 @@
 
       if (g->global_gotsym != NULL)
 	BFD_ASSERT ((elf_hash_table (info)->dynsymcount
-		     - g->global_gotsym->dynindx)
+		     - g->global_gotsym->dynindx
+		     - g->forced_local_count)
 		    <= g->global_gotno);
     }
 
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.47
diff -u -r1.47 mips-elf.exp
--- ld/testsuite/ld-mips-elf/mips-elf.exp	13 Aug 2007 21:16:39 -0000	1.47
+++ ld/testsuite/ld-mips-elf/mips-elf.exp	28 Sep 2007 22:40:44 -0000
@@ -68,6 +68,8 @@
 if { $linux_gnu } {
     run_dump_test "multi-got-1"
     run_dump_test "multi-got-no-shared"
+    run_dump_test "multi-got-hidden-1"
+    run_dump_test "multi-got-hidden-2"
 }
 
 if $has_newabi {
Index: ld/testsuite/ld-mips-elf/multi-got-hidden-1.d
===================================================================
RCS file: ld/testsuite/ld-mips-elf/multi-got-hidden-1.d
diff -N ld/testsuite/ld-mips-elf/multi-got-hidden-1.d
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-mips-elf/multi-got-hidden-1.d	28 Sep 2007 22:40:44 -0000
@@ -0,0 +1,8 @@
+#name: MIPS multi-got-hidden-1
+#as: -EB -32 -KPIC
+#source: multi-got-1-1.s
+#source: multi-got-1-2.s
+#source: multi-got-hidden-1.s
+#ld: -melf32btsmip -e 0
+#objdump: -dr
+#pass
Index: ld/testsuite/ld-mips-elf/multi-got-hidden-1.s
===================================================================
RCS file: ld/testsuite/ld-mips-elf/multi-got-hidden-1.s
diff -N ld/testsuite/ld-mips-elf/multi-got-hidden-1.s
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-mips-elf/multi-got-hidden-1.s	28 Sep 2007 22:40:44 -0000
@@ -0,0 +1,5 @@
+.hidden __init_array_end
+.hidden __init_array_start
+sym_3_1:
+la $2, __init_array_start
+la $2, __init_array_end
Index: ld/testsuite/ld-mips-elf/multi-got-hidden-2.d
===================================================================
RCS file: ld/testsuite/ld-mips-elf/multi-got-hidden-2.d
diff -N ld/testsuite/ld-mips-elf/multi-got-hidden-2.d
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ ld/testsuite/ld-mips-elf/multi-got-hidden-2.d	28 Sep 2007 22:40:44 -0000
@@ -0,0 +1,7 @@
+#name: MIPS multi-got-hidden-2
+#as: -EB -32 -KPIC
+#source: multi-got-1-1.s
+#source: multi-got-hidden-2.s
+#ld: -melf32btsmip -e 0
+#objdump: -dr
+#pass

-- 
Joseph S. Myers
joseph@codesourcery.com

Ping^2 Re: Ping Re: Patch for MIPS multi-got bug with forced-local symbols

[Joseph S. Myers]

Ping (again).  Neither this revised patch 
<http://sourceware.org/ml/binutils/2007-09/msg00436.html> nor the original 
version has been reviewed.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Ping Re: Patch for MIPS multi-got bug with forced-local symbols

[Daniel Jacobowitz]

On Fri, Sep 28, 2007 at 10:52:34PM +0000, Joseph S. Myers wrote:
> @@ -2757,8 +2764,10 @@
>         referenced, we move them to the end of the GOT, so that they
>         don't prevent other entries that are referenced from getting
>         too large offsets.  */
> -    - (g->next ? g->assigned_gotno : 0);
> +    - (g->next ? g->assigned_gotno : 0) - g->forced_local_count;

This definitely needs some explanation in the comment above.  I can't
figure out why it's right.

> -      h->root.dynindx = --hsd->min_got_dynindx;
> -      hsd->low = (struct elf_link_hash_entry *) h;
> +      if (h->forced_local && hsd->forced_local <= hsd->prev_forced_local)
> +	h->root.dynindx = hsd->max_unref_got_dynindx++;
> +      else
> +	{
> +	  h->root.dynindx = --hsd->min_got_dynindx;
> +	  hsd->low = (struct elf_link_hash_entry *) h;
> +	}

Here you're putting some symbols at the end of the GOT... but where's
the space for them supposed to come from in the non-multi-GOT case?
They end up not included in global_gotno, so GOT entries for
__init_array_start and __init_array_end are created at runtime in the
two words following the GOT.  I suspect you can reproduce this with
any program that doesn't use multi-GOT.  Just look at the size of the
GOT and compare with sizeof (void *) * (DT_SYMTABNO - DT_GOTSYM).

I will keep investigating.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Ping^2 Re: Ping Re: Patch for MIPS multi-got bug with forced-local symbols

[Nick Clifton]

Hi Joseph,

> Ping (again).  Neither this revised patch 
> <http://sourceware.org/ml/binutils/2007-09/msg00436.html> nor the original 
> version has been reviewed.

I have not heard any objections from the MIPS maintainers, so please consider 
this patch approved.

Cheers
   Nick

Re: Ping^2 Re: Ping Re: Patch for MIPS multi-got bug with forced-local symbols

[Daniel Jacobowitz]

On Mon, Oct 08, 2007 at 11:46:22AM +0100, Nick Clifton wrote:
> Hi Joseph,
> 
> > Ping (again).  Neither this revised patch 
> > <http://sourceware.org/ml/binutils/2007-09/msg00436.html> nor the original 
> > version has been reviewed.
> 
> I have not heard any objections from the MIPS maintainers, so please consider 
> this patch approved.

You missed my objection on Saturday :-)

I'm still looking into the problem, but this patch breaks normal
(non-multi-GOT) executables.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Ping^2 Re: Ping Re: Patch for MIPS multi-got bug with forced-local symbols

[Nick Clifton]

Hi Daniel,

>> I have not heard any objections from the MIPS maintainers, so please consider 
>> this patch approved.
> 
> You missed my objection on Saturday :-)

Doh!  Sorry.

Cheers
   Nick

Re: Ping Re: Patch for MIPS multi-got bug with forced-local symbols

[Daniel Jacobowitz]

From struct mips_elf_link_hash_entry:

  /* Are we forced local?  This will only be set if we have converted
     the initial global GOT entry to a local GOT entry.  */
  bfd_boolean forced_local;

Current binutils is not really doing that conversion.  We allocate a
local GOT entry, but never use it.  When _bfd_mips_elf_hide_symbol
is called for PROVIDE_HIDDEN (__init_array_start), it increments
local_gotno.  But dynsymcount includes forced local symbols, and they
are placed above dynindx by mips_elf_sort_hash_table_f, so they
end up with entries on the global side.

mips_elf_sort_hash_table_f gave it a symbol index above dynindx
because it had got.offset == 1.  Joseph earlier established that
this happens in two places: initially in
mips_elf_record_global_got_symbol, and later to 2 and then to 1
in mips_elf_set_global_got_offset.

I see two solutions.  One is to avoid setting got.offset for forced
local symbols, and set it back to MINUS_ONE when hiding symbols.
The other is to set got.offset but still not put such symbols in the
global GOT when the time comes.

The second is easier, since we use got.offset != MINUS_ONE in
mips_elf_record_global_got_symbol to detect symbols with already
allocated GOT entries.  This patch implements that.  It fixes
Joseph's two new testcases, which should be committed along
with it.  I'm going to start more intensive testing now.

Anyone have comments on this patch?

-- 
Daniel Jacobowitz
CodeSourcery

2007-10-08  Daniel Jacobowitz  <dan@codesourcery.com>

	* elfxx-mips.c (mips_elf_sort_hash_table_f): Handle forced
	local symbols specially.
	(mips_elf_set_global_got_offset): Skip forced local symbols.

--- elfxx-mips.c.pre	2007-10-08 05:44:21.000000000 -0700
+++ elfxx-mips.c	2007-10-08 06:20:14.000000000 -0700
@@ -2842,7 +2842,8 @@ mips_elf_sort_hash_table_f (struct mips_
   /* Global symbols that need GOT entries that are not explicitly
      referenced are marked with got offset 2.  Those that are
      referenced get a 1, and those that don't need GOT entries get
-     -1.  */
+     -1.  Forced local symbols may also be marked with got offset 1,
+     but are never given global GOT entries.  */
   if (h->root.got.offset == 2)
     {
       BFD_ASSERT (h->tls_type == GOT_NORMAL);
@@ -2851,7 +2852,7 @@ mips_elf_sort_hash_table_f (struct mips_
 	hsd->low = (struct elf_link_hash_entry *) h;
       h->root.dynindx = hsd->max_unref_got_dynindx++;
     }
-  else if (h->root.got.offset != 1)
+  else if (h->root.got.offset != 1 || h->root.forced_local)
     h->root.dynindx = hsd->max_non_got_dynindx++;
   else
     {
@@ -3459,6 +3460,7 @@ mips_elf_set_global_got_offset (void **e
 
   if (entry->abfd != NULL && entry->symndx == -1
       && entry->d.h->root.dynindx != -1
+      && !entry->d.h->forced_local
       && entry->d.h->tls_type == GOT_NORMAL)
     {
       if (g)

Re: Ping Re: Patch for MIPS multi-got bug with forced-local symbols

[Daniel Jacobowitz]

On Mon, Oct 08, 2007 at 09:36:25AM -0400, Daniel Jacobowitz wrote:
> -  else if (h->root.got.offset != 1)
> +  else if (h->root.got.offset != 1 || h->root.forced_local)

That should be h->forced_local.  The difference between these two
variables is confusing and there doesn't seem to be a clear
explanation of which is which; but h->forced_local means "has
been through the backend hide_symbol routine" which affects
accounting of GOT entries.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Ping Re: Patch for MIPS multi-got bug with forced-local symbols

[Richard Sandiford]

Daniel Jacobowitz <drow@false.org> writes:
> I see two solutions.  One is to avoid setting got.offset for forced
> local symbols, and set it back to MINUS_ONE when hiding symbols.
> The other is to set got.offset but still not put such symbols in the
> global GOT when the time comes.
>
> The second is easier, since we use got.offset != MINUS_ONE in
> mips_elf_record_global_got_symbol to detect symbols with already
> allocated GOT entries.  This patch implements that.  It fixes
> Joseph's two new testcases, which should be committed along
> with it.  I'm going to start more intensive testing now.
>
> Anyone have comments on this patch?

FWIW, it looks good to me.  I think the second solution is indeed how
things are supposed to work at the moment, and if we're going to change
the datastructures, a bigger overhaul than the first option might be
useful...

Richard

Re: Ping Re: Patch for MIPS multi-got bug with forced-local symbols

[Daniel Jacobowitz]

On Tue, Oct 09, 2007 at 09:35:09PM +0100, Richard Sandiford wrote:
> FWIW, it looks good to me.  I think the second solution is indeed how
> things are supposed to work at the moment, and if we're going to change
> the datastructures, a bigger overhaul than the first option might be
> useful...

Thanks.  Here's what I have checked in.

-- 
Daniel Jacobowitz
CodeSourcery

2007-10-12  Daniel Jacobowitz  <dan@codesourcery.com>

	* elfxx-mips.c (mips_elf_sort_hash_table_f): Handle forced
	local symbols specially.
	(mips_elf_set_global_got_offset): Skip forced local symbols.

--- elfxx-mips.c.pre	2007-10-08 05:44:21.000000000 -0700
+++ elfxx-mips.c	2007-10-08 06:20:14.000000000 -0700
@@ -2842,7 +2842,8 @@ mips_elf_sort_hash_table_f (struct mips_
   /* Global symbols that need GOT entries that are not explicitly
      referenced are marked with got offset 2.  Those that are
      referenced get a 1, and those that don't need GOT entries get
-     -1.  */
+     -1.  Forced local symbols may also be marked with got offset 1,
+     but are never given global GOT entries.  */
   if (h->root.got.offset == 2)
     {
       BFD_ASSERT (h->tls_type == GOT_NORMAL);
@@ -2851,7 +2852,7 @@ mips_elf_sort_hash_table_f (struct mips_
 	hsd->low = (struct elf_link_hash_entry *) h;
       h->root.dynindx = hsd->max_unref_got_dynindx++;
     }
-  else if (h->root.got.offset != 1)
+  else if (h->root.got.offset != 1 || h->forced_local)
     h->root.dynindx = hsd->max_non_got_dynindx++;
   else
     {
@@ -3459,6 +3460,7 @@ mips_elf_set_global_got_offset (void **e
 
   if (entry->abfd != NULL && entry->symndx == -1
       && entry->d.h->root.dynindx != -1
+      && !entry->d.h->forced_local
       && entry->d.h->tls_type == GOT_NORMAL)
     {
       if (g)

Re: Ping Re: Patch for MIPS multi-got bug with forced-local symbols

[Martin Michlmayr]

* Daniel Jacobowitz <drow@false.org> [2007-10-12 11:59]:
> > FWIW, it looks good to me.  I think the second solution is indeed how
> > things are supposed to work at the moment, and if we're going to change
> > the datastructures, a bigger overhaul than the first option might be
> > useful...
> Thanks.  Here's what I have checked in.

This fixes PR 4988.  Can you put this into 2.18?

> -- 
> Daniel Jacobowitz
> CodeSourcery
> 
> 2007-10-12  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* elfxx-mips.c (mips_elf_sort_hash_table_f): Handle forced
> 	local symbols specially.
> 	(mips_elf_set_global_got_offset): Skip forced local symbols.
> 
> --- elfxx-mips.c.pre	2007-10-08 05:44:21.000000000 -0700
> +++ elfxx-mips.c	2007-10-08 06:20:14.000000000 -0700
> @@ -2842,7 +2842,8 @@ mips_elf_sort_hash_table_f (struct mips_
>    /* Global symbols that need GOT entries that are not explicitly
>       referenced are marked with got offset 2.  Those that are
>       referenced get a 1, and those that don't need GOT entries get
> -     -1.  */
> +     -1.  Forced local symbols may also be marked with got offset 1,
> +     but are never given global GOT entries.  */
>    if (h->root.got.offset == 2)
>      {
>        BFD_ASSERT (h->tls_type == GOT_NORMAL);
> @@ -2851,7 +2852,7 @@ mips_elf_sort_hash_table_f (struct mips_
>  	hsd->low = (struct elf_link_hash_entry *) h;
>        h->root.dynindx = hsd->max_unref_got_dynindx++;
>      }
> -  else if (h->root.got.offset != 1)
> +  else if (h->root.got.offset != 1 || h->forced_local)
>      h->root.dynindx = hsd->max_non_got_dynindx++;
>    else
>      {
> @@ -3459,6 +3460,7 @@ mips_elf_set_global_got_offset (void **e
>  
>    if (entry->abfd != NULL && entry->symndx == -1
>        && entry->d.h->root.dynindx != -1
> +      && !entry->d.h->forced_local
>        && entry->d.h->tls_type == GOT_NORMAL)
>      {
>        if (g)

-- 
Martin Michlmayr
http://www.cyrius.com/

Re: Ping Re: Patch for MIPS multi-got bug with forced-local symbols

[Nick Clifton]

Hi Martin,

> This fixes PR 4988.  Can you put this into 2.18?

>> 2007-10-12  Daniel Jacobowitz  <dan@codesourcery.com>
>>
>> 	* elfxx-mips.c (mips_elf_sort_hash_table_f): Handle forced
>> 	local symbols specially.
>> 	(mips_elf_set_global_got_offset): Skip forced local symbols.

It is already in the branch, although strangely the ChangeLog entry did not 
make it in.  I will commit the ChangeLog entry today.

Cheers
   Nick

Re: Ping Re: Patch for MIPS multi-got bug with forced-local symbols

[Daniel Jacobowitz]

On Sat, Oct 27, 2007 at 09:56:33AM +0100, Nick Clifton wrote:
> Hi Martin,
> 
> > This fixes PR 4988.  Can you put this into 2.18?
> 
> >> 2007-10-12  Daniel Jacobowitz  <dan@codesourcery.com>
> >>
> >> 	* elfxx-mips.c (mips_elf_sort_hash_table_f): Handle forced
> >> 	local symbols specially.
> >> 	(mips_elf_set_global_got_offset): Skip forced local symbols.
> 
> It is already in the branch, although strangely the ChangeLog entry did not 
> make it in.  I will commit the ChangeLog entry today.

I added it yesterday after his message.  (Sorry for the confusion -
Martin also asked in Bugzilla and I replied there.)

I added the changelog entry in the same relative place that it was on
mainline.  In hindsight maybe that's not a good idea.  Should I use
the date I committed it to the branch instead?

-- 
Daniel Jacobowitz
CodeSourcery

Re: Ping Re: Patch for MIPS multi-got bug with forced-local symbols

[Nick Clifton]

Hi Daniel,

> I added the changelog entry in the same relative place that it was on
> mainline.  In hindsight maybe that's not a good idea.  Should I use
> the date I committed it to the branch instead?

Yes please.  I think that the date ought to reflect the point at which the 
patch was applied to the (branch/mainline) sources, not the time at which the 
patch was actually developed.  This makes it easier for someone to use CVS's -D 
command line option to isolate the patch should they need to locate it.

Cheers
   Nick