RFA: Adjust MIPS hidden symbol handling

RFA: Adjust MIPS hidden symbol handling

[Daniel Jacobowitz]

The MIPS backend currently has its own state for forced local symbols.  But
it's redundant with the common set, and the three places we check that flag
are bogus for the other sources of hidden symbols besides hide_symbol.

Fixes several assertion failures on mips64-linux using TLS, no other changes
in the binutils testsuite.  OK?

-- 
Daniel Jacobowitz
CodeSourcery, LLC

2005-04-13  Daniel Jacobowitz  <dan@codesourcery.com>

	* elfxx-mips.c (struct mips_got_entry): Update comment.
	(struct mips_elf_link_hash_entry): Remove forced_local.
	(mips_elf_link_hash_newfunc): Don't initialize forced_local.
	(mips_elf_create_local_got_entry): Check h->root.forced_local
	instead.
	(mips_elf_make_got_per_bfd): Likewise.
	(_bfd_mips_elf_hide_symbol): Likewise.  Don't set forced_local.

Index: binutils/bfd/elfxx-mips.c
===================================================================
--- binutils.orig/bfd/elfxx-mips.c	2005-04-12 16:39:56.000000000 -0400
+++ binutils/bfd/elfxx-mips.c	2005-04-13 13:38:18.360504220 -0400
@@ -61,7 +61,7 @@ struct mips_got_entry
     bfd_vma addend;
     /* If abfd != NULL && symndx == -1, the hash table entry
        corresponding to a global symbol in the got (or, local, if
-       h->forced_local).  */
+       h->root.forced_local).  */
     struct mips_elf_link_hash_entry *h;
   } d;
 
@@ -242,9 +242,6 @@ struct mips_elf_link_hash_entry
      being called returns a floating point value.  */
   asection *call_fp_stub;
 
-  /* Are we forced local?  .*/
-  bfd_boolean forced_local;
-
 #define GOT_NORMAL	0
 #define GOT_TLS_GD	1
 #define GOT_TLS_LDM	2
@@ -714,7 +711,6 @@ mips_elf_link_hash_newfunc (struct bfd_h
       ret->need_fn_stub = FALSE;
       ret->call_stub = NULL;
       ret->call_fp_stub = NULL;
-      ret->forced_local = FALSE;
       ret->tls_type = GOT_NORMAL;
     }
 
@@ -2463,7 +2459,7 @@ mips_elf_create_local_got_entry (bfd *ab
      global entry then.  It doesn't matter whether an entry is local
      or global for TLS, since the dynamic linker does not
      automatically relocate TLS GOT entries.  */
-  BFD_ASSERT (h == NULL || h->forced_local);
+  BFD_ASSERT (h == NULL || h->root.forced_local);
   if (TLS_RELOC_P (r_type))
     {
       struct mips_got_entry *p;
@@ -2855,7 +2851,7 @@ mips_elf_make_got_per_bfd (void **entryp
       if (entry->tls_type & GOT_TLS_IE)
 	g->tls_gotno += 1;
     }
-  else if (entry->symndx >= 0 || entry->d.h->forced_local)
+  else if (entry->symndx >= 0 || entry->d.h->root.forced_local)
     ++g->local_gotno;
   else
     ++g->global_gotno;
@@ -8356,9 +8352,8 @@ _bfd_mips_elf_hide_symbol (struct bfd_li
   struct mips_elf_link_hash_entry *h;
 
   h = (struct mips_elf_link_hash_entry *) entry;
-  if (h->forced_local)
+  if (h->root.forced_local)
     return;
-  h->forced_local = force_local;
 
   dynobj = elf_hash_table (info)->dynobj;
   if (dynobj != NULL && force_local && h->root.type != STT_TLS)

Re: RFA: Adjust MIPS hidden symbol handling

[Thiemo Seufer]

Daniel Jacobowitz wrote:
[snip]
> 2005-04-13  Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* elfxx-mips.c (struct mips_got_entry): Update comment.
> 	(struct mips_elf_link_hash_entry): Remove forced_local.
> 	(mips_elf_link_hash_newfunc): Don't initialize forced_local.
> 	(mips_elf_create_local_got_entry): Check h->root.forced_local
> 	instead.
> 	(mips_elf_make_got_per_bfd): Likewise.
> 	(_bfd_mips_elf_hide_symbol): Likewise.  Don't set forced_local.

Ok.


Thiemo

Re: RFA: Adjust MIPS hidden symbol handling

[Daniel Jacobowitz]

On Wed, Apr 13, 2005 at 07:21:16PM +0200, Thiemo Seufer wrote:
> Daniel Jacobowitz wrote:
> [snip]
> > 2005-04-13  Daniel Jacobowitz  <dan@codesourcery.com>
> > 
> > 	* elfxx-mips.c (struct mips_got_entry): Update comment.
> > 	(struct mips_elf_link_hash_entry): Remove forced_local.
> > 	(mips_elf_link_hash_newfunc): Don't initialize forced_local.
> > 	(mips_elf_create_local_got_entry): Check h->root.forced_local
> > 	instead.
> > 	(mips_elf_make_got_per_bfd): Likewise.
> > 	(_bfd_mips_elf_hide_symbol): Likewise.  Don't set forced_local.
> 
> Ok.

Unfortunately, not.

I still think the patch is right in principle, and without it I get all
sorts of assertion failures - but with it, I don't get enough room
allocated for local GOT entries.

What I think is happening is that without the patch, several kinds of
hidden symbols are given unnecessary (and probably undesirable) global
GOT entries.  With it, they're given local GOT entries - but something
else needs to be adjusted to account for it.

I'm going to keep looking into this :-(  I'd _like_ to fix it for 2.16,
but 2.16 is running late already.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: RFA: Adjust MIPS hidden symbol handling

[Daniel Jacobowitz]

On Thu, Apr 14, 2005 at 10:35:25PM -0400, Daniel Jacobowitz wrote:
> On Wed, Apr 13, 2005 at 07:21:16PM +0200, Thiemo Seufer wrote:
> > Daniel Jacobowitz wrote:
> > [snip]
> > > 2005-04-13  Daniel Jacobowitz  <dan@codesourcery.com>
> > > 
> > > 	* elfxx-mips.c (struct mips_got_entry): Update comment.
> > > 	(struct mips_elf_link_hash_entry): Remove forced_local.
> > > 	(mips_elf_link_hash_newfunc): Don't initialize forced_local.
> > > 	(mips_elf_create_local_got_entry): Check h->root.forced_local
> > > 	instead.
> > > 	(mips_elf_make_got_per_bfd): Likewise.
> > > 	(_bfd_mips_elf_hide_symbol): Likewise.  Don't set forced_local.
> > 
> > Ok.
> 
> Unfortunately, not.
> 
> I still think the patch is right in principle, and without it I get all
> sorts of assertion failures - but with it, I don't get enough room
> allocated for local GOT entries.
> 
> What I think is happening is that without the patch, several kinds of
> hidden symbols are given unnecessary (and probably undesirable) global
> GOT entries.  With it, they're given local GOT entries - but something
> else needs to be adjusted to account for it.
> 
> I'm going to keep looking into this :-(  I'd _like_ to fix it for 2.16,
> but 2.16 is running late already.

Nah, I was wrong.  The two flags aren't quite the same: the
mips-specific flag is used for accounting when we convert a global GOT
entry to a local one.  So only the assertion is wrong. 
h->forced_local implies h->root.forced_local, but not the other way
around.

I'm committing this patch.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

2005-04-19  Daniel Jacobowitz  <dan@codesourcery.com>

	* elfxx-mips.c (struct mips_elf_link_hash_entry): Update comment.
	(mips_elf_create_local_got_entry): Check h->root.forced_local.

Index: binutils/bfd/elfxx-mips.c
===================================================================
--- binutils.orig/bfd/elfxx-mips.c	2005-04-19 12:36:30.830426877 -0400
+++ binutils/bfd/elfxx-mips.c	2005-04-19 12:36:58.795727677 -0400
@@ -242,7 +242,8 @@ struct mips_elf_link_hash_entry
      being called returns a floating point value.  */
   asection *call_fp_stub;
 
-  /* Are we forced local?  .*/
+  /* 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;
 
 #define GOT_NORMAL	0
@@ -2463,7 +2464,7 @@ mips_elf_create_local_got_entry (bfd *ab
      global entry then.  It doesn't matter whether an entry is local
      or global for TLS, since the dynamic linker does not
      automatically relocate TLS GOT entries.  */
-  BFD_ASSERT (h == NULL || h->forced_local);
+  BFD_ASSERT (h == NULL || h->root.forced_local);
   if (TLS_RELOC_P (r_type))
     {
       struct mips_got_entry *p;

Re: RFA: Adjust MIPS hidden symbol handling

[Maciej W. Rozycki]

On Tue, 19 Apr 2005, Daniel Jacobowitz wrote:

> Nah, I was wrong.  The two flags aren't quite the same: the
> mips-specific flag is used for accounting when we convert a global GOT
> entry to a local one.  So only the assertion is wrong. 
> h->forced_local implies h->root.forced_local, but not the other way
> around.

 This sounds like the correct interpretation -- now that you've mentioned 
that GOT reference I've recalled doing some work in that area long ago 
when support for linker scripts (and converting symbols to local this way) 
was added to the MIPS backend.  I can have a look at the relevant code 
again if you discover any further problems.

  Maciej

Re: RFA: Adjust MIPS hidden symbol handling

[Daniel Jacobowitz]

On Tue, Apr 19, 2005 at 07:51:02PM +0100, Maciej W. Rozycki wrote:
> On Tue, 19 Apr 2005, Daniel Jacobowitz wrote:
> 
> > Nah, I was wrong.  The two flags aren't quite the same: the
> > mips-specific flag is used for accounting when we convert a global GOT
> > entry to a local one.  So only the assertion is wrong. 
> > h->forced_local implies h->root.forced_local, but not the other way
> > around.
> 
>  This sounds like the correct interpretation -- now that you've mentioned 
> that GOT reference I've recalled doing some work in that area long ago 
> when support for linker scripts (and converting symbols to local this way) 
> was added to the MIPS backend.  I can have a look at the relevant code 
> again if you discover any further problems.

Thanks, that fits.  One big difference I encountered between the two
cases is that symbols added by a linker script are never passed to the
backend hide_symbol function.

-- 
Daniel Jacobowitz
CodeSourcery, LLC