RFC: Fix ia64 visibility

RFC: Fix ia64 visibility

[H. J. Lu]

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

On ia64, when we hide a symbol, we clear the want_plt2 field, but not
want_plt. I don't see a reason not to. Also, if we don't need dynamic
symbol lookup, which is true for symbols with non-default visibility,
we should avoid it. Is this patch OK?


H.J.

[-- Attachment #2: bfd-ia64-protected.patch --]
[-- Type: text/plain, Size: 1381 bytes --]

2003-05-06  H.J. Lu <hongjiu.lu@intel.com>

	* elfxx-ia64.c (_bfd_elf_link_hash_hide_symbol): Also clear the
	want_plt field.
	(elfNN_ia64_relocate_section): Don't do dynamic symbol lookup
	for symbols with non-default visibility.

--- bfd/elfxx-ia64.c.hjl	2003-05-05 08:27:08.000000000 -0700
+++ bfd/elfxx-ia64.c	2003-05-06 15:42:59.000000000 -0700
@@ -1792,7 +1792,10 @@ elfNN_ia64_hash_hide_symbol (info, xh, f
   _bfd_elf_link_hash_hide_symbol (info, &h->root, force_local);
 
   for (dyn_i = h->info; dyn_i; dyn_i = dyn_i->next)
-    dyn_i->want_plt2 = 0;
+    {
+      dyn_i->want_plt2 = 0;
+      dyn_i->want_plt = 0;
+    }
 }
 
 /* Create the derived linker hash table.  The IA-64 ELF port uses this
@@ -4033,7 +4036,8 @@ elfNN_ia64_relocate_section (output_bfd,
 	      /* If we don't need dynamic symbol lookup, find a
 		 matching RELATIVE relocation.  */
 	      dyn_r_type = r_type;
-	      if (dynamic_symbol_p)
+	      if (dynamic_symbol_p
+		  && ELF_ST_VISIBILITY (h->other) == STV_DEFAULT)
 		{
 		  dynindx = h->dynindx;
 		  addend = rel->r_addend;
@@ -4362,7 +4366,8 @@ elfNN_ia64_relocate_section (output_bfd,
 
 	      /* If we don't need dynamic symbol lookup, install two
 		 RELATIVE relocations.  */
-	      if (! dynamic_symbol_p)
+	      if (! dynamic_symbol_p
+		  || ELF_ST_VISIBILITY (h->other) != STV_DEFAULT)
 		{
 		  unsigned int dyn_r_type;
 

Re: RFC: Fix ia64 visibility

[Richard Henderson]

On Tue, May 06, 2003 at 04:04:02PM -0700, H. J. Lu wrote:
> -    dyn_i->want_plt2 = 0;
> +    {
> +      dyn_i->want_plt2 = 0;
> +      dyn_i->want_plt = 0;
> +    }

You've not checked that there are no PLTOFF relocations.

> -	      if (dynamic_symbol_p)
> +	      if (dynamic_symbol_p
> +		  && ELF_ST_VISIBILITY (h->other) == STV_DEFAULT)

dynamic_symbol_p has already checked visibility.  Why
would we need to do it again?


r~

Re: RFC: Fix ia64 visibility

[H. J. Lu]

On Wed, May 07, 2003 at 11:09:32AM -0700, Richard Henderson wrote:
> On Tue, May 06, 2003 at 04:04:02PM -0700, H. J. Lu wrote:
> > -    dyn_i->want_plt2 = 0;
> > +    {
> > +      dyn_i->want_plt2 = 0;
> > +      dyn_i->want_plt = 0;
> > +    }
> 
> You've not checked that there are no PLTOFF relocations.

I thought it was checked by the want_pltoff field.

> 
> > -	      if (dynamic_symbol_p)
> > +	      if (dynamic_symbol_p
> > +		  && ELF_ST_VISIBILITY (h->other) == STV_DEFAULT)
> 
> dynamic_symbol_p has already checked visibility.  Why
> would we need to do it again?

dynamic_symbol_p only checks STV_INTERNAL and STV_HIDDEN. For certain
relocation types, there is no need for dynamic symbol lookup even for
STV_PROTECTED. I can use ELF_ST_VISIBILITY (h->other) != STV_PROTECTED.
But I thought ELF_ST_VISIBILITY (h->other) == STV_DEFAULT might help
compiler a little bit. Maybe I should put a comment there.


H.J.

Re: RFC: Fix ia64 visibility

[Richard Henderson]

On Wed, May 07, 2003 at 11:19:46AM -0700, H. J. Lu wrote:
> I thought it was checked by the want_pltoff field.

Ah yes.  Hmm, I guess this part is ok then.

> dynamic_symbol_p only checks STV_INTERNAL and STV_HIDDEN. For certain
> relocation types, there is no need for dynamic symbol lookup even for
> STV_PROTECTED. I can use ELF_ST_VISIBILITY (h->other) != STV_PROTECTED.
> But I thought ELF_ST_VISIBILITY (h->other) == STV_DEFAULT might help
> compiler a little bit. Maybe I should put a comment there.

I'd rather we not sprinkle STV_PROTECTED checks (or anything else)
all over the place.  Is it actually incorrect to modify dynamic_symbol_p
to check STV_PROTECTED?  If so, we should have a different predicate
function that tests exactly what we want and use that.


r~

Re: RFC: Fix ia64 visibility

[H. J. Lu]

On Wed, May 07, 2003 at 03:30:34PM -0700, Richard Henderson wrote:
> On Wed, May 07, 2003 at 11:19:46AM -0700, H. J. Lu wrote:
> 
> > dynamic_symbol_p only checks STV_INTERNAL and STV_HIDDEN. For certain
> > relocation types, there is no need for dynamic symbol lookup even for
> > STV_PROTECTED. I can use ELF_ST_VISIBILITY (h->other) != STV_PROTECTED.
> > But I thought ELF_ST_VISIBILITY (h->other) == STV_DEFAULT might help
> > compiler a little bit. Maybe I should put a comment there.
> 
> I'd rather we not sprinkle STV_PROTECTED checks (or anything else)
> all over the place.  Is it actually incorrect to modify dynamic_symbol_p
> to check STV_PROTECTED?  If so, we should have a different predicate
> function that tests exactly what we want and use that.

It is an optimization for calling directly to protected functions. In
that case, we don't need PLT nor function descriptors. It is the same
as calling an internal function. But we can't do it for function
descriptors of protected functions. That check is based on relocation
type, not just symbol alone. Another way to do it is to pass reloction
type to elfNN_ia64_dynamic_symbol_p so that it has enough info to do
the right thing. I can submit a new patch if it is preferred.


H.J.

Re: RFC: Fix ia64 visibility

[Jim Wilson]

H. J. Lu wrote:
> On ia64, when we hide a symbol, we clear the want_plt2 field, but not
> want_plt. I don't see a reason not to. Also, if we don't need dynamic
> symbol lookup, which is true for symbols with non-default visibility,
> we should avoid it. Is this patch OK?

bfd is mostly a black box to me, but this stuff seems reasonable to me 
with your explanation.

want_plt2 is for dynamic symbols which are functions which need a full 
plt.  want_plt is for dynamic symbols which are not functions which need 
a minimal plt.  want_pltoff is for symbols, dynamic or not, which have 
pltoff relocations.  If a symbol is hidden, then we no longer need 
dynamic relocs, and hence want_plt should be cleared along with want_plt2.

The other bit is code that is simplying relocs if we don't need dynamic 
symbol lookup, which is the case for protected symbols, so that makes 
sense too.

Jim

Re: RFC: Fix ia64 visibility

[Richard Henderson]

On Wed, May 07, 2003 at 04:15:08PM -0700, H. J. Lu wrote:
> It is an optimization for calling directly to protected functions. In
> that case, we don't need PLT nor function descriptors. It is the same
> as calling an internal function.

Certainly.

> But we can't do it for function
> descriptors of protected functions. That check is based on relocation
> type, not just symbol alone.

Because ld.so is going to allocate it, correct?

> Another way to do it is to pass reloction type to
> elfNN_ia64_dynamic_symbol_p so that it has enough info to do
> the right thing.

Eh.  I'd prefer a different function.  It can do little else
besides checking the reloc plus calling elfNN_ia64_dynamic_symbol_p.

> I can submit a new patch if it is preferred.

Please.


r~

Re: RFC: Fix ia64 visibility

[H. J. Lu]

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

On Wed, May 07, 2003 at 04:17:15PM -0700, Richard Henderson wrote:
> On Wed, May 07, 2003 at 04:15:08PM -0700, H. J. Lu wrote:
> > It is an optimization for calling directly to protected functions. In
> > that case, we don't need PLT nor function descriptors. It is the same
> > as calling an internal function.
> 
> Certainly.
> 
> > But we can't do it for function
> > descriptors of protected functions. That check is based on relocation
> > type, not just symbol alone.
> 
> Because ld.so is going to allocate it, correct?

Yes.

> 
> > Another way to do it is to pass reloction type to
> > elfNN_ia64_dynamic_symbol_p so that it has enough info to do
> > the right thing.
> 
> Eh.  I'd prefer a different function.  It can do little else
> besides checking the reloc plus calling elfNN_ia64_dynamic_symbol_p.
> 
> > I can submit a new patch if it is preferred.

I didn't create a new function since it isn't used anywhere else.
Instead, I addded a new variable and set it based on the return
value of elfNN_ia64_dynamic_symbol_p and symbol visibility.


H.J.

[-- Attachment #2: bfd-ia64-protected.patch --]
[-- Type: text/plain, Size: 2023 bytes --]

2003-05-07  H.J. Lu <hongjiu.lu@intel.com>

	* elfxx-ia64.c (_bfd_elf_link_hash_hide_symbol): Also clear the
	want_plt field.
	(elfNN_ia64_relocate_section): Don't do dynamic symbol lookup
	for symbols with non-default visibility.

--- bfd/elfxx-ia64.c.ia64	2003-05-05 08:27:08.000000000 -0700
+++ bfd/elfxx-ia64.c	2003-05-07 22:46:17.000000000 -0700
@@ -1792,7 +1792,10 @@ elfNN_ia64_hash_hide_symbol (info, xh, f
   _bfd_elf_link_hash_hide_symbol (info, &h->root, force_local);
 
   for (dyn_i = h->info; dyn_i; dyn_i = dyn_i->next)
-    dyn_i->want_plt2 = 0;
+    {
+      dyn_i->want_plt2 = 0;
+      dyn_i->want_plt = 0;
+    }
 }
 
 /* Create the derived linker hash table.  The IA-64 ELF port uses this
@@ -3890,6 +3893,7 @@ elfNN_ia64_relocate_section (output_bfd,
       asection *sym_sec;
       bfd_byte *hit_addr;
       bfd_boolean dynamic_symbol_p;
+      bfd_boolean local_symbol_p;
       bfd_boolean undef_weak_ref;
 
       r_type = ELFNN_R_TYPE (rel->r_info);
@@ -4001,6 +4005,11 @@ elfNN_ia64_relocate_section (output_bfd,
       hit_addr = contents + rel->r_offset;
       value += rel->r_addend;
       dynamic_symbol_p = elfNN_ia64_dynamic_symbol_p (h, info);
+      /* Is this symbol locally defined? A protected symbol is locallly
+	 defined. But its function descriptor may not. Use it with
+	 caution.  */
+      local_symbol_p = (! dynamic_symbol_p
+			|| ELF_ST_VISIBILITY (h->other) != STV_DEFAULT);
 
       switch (r_type)
 	{
@@ -4033,7 +4042,7 @@ elfNN_ia64_relocate_section (output_bfd,
 	      /* If we don't need dynamic symbol lookup, find a
 		 matching RELATIVE relocation.  */
 	      dyn_r_type = r_type;
-	      if (dynamic_symbol_p)
+	      if (! local_symbol_p)
 		{
 		  dynindx = h->dynindx;
 		  addend = rel->r_addend;
@@ -4362,7 +4371,7 @@ elfNN_ia64_relocate_section (output_bfd,
 
 	      /* If we don't need dynamic symbol lookup, install two
 		 RELATIVE relocations.  */
-	      if (! dynamic_symbol_p)
+	      if (local_symbol_p)
 		{
 		  unsigned int dyn_r_type;
 

Re: RFC: Fix ia64 visibility

[Richard Henderson]

On Wed, May 07, 2003 at 10:51:53PM -0700, H. J. Lu wrote:
> I didn't create a new function since it isn't used anywhere else.

Yet.

> 	* elfxx-ia64.c (_bfd_elf_link_hash_hide_symbol): Also clear the
> 	want_plt field.
> 	(elfNN_ia64_relocate_section): Don't do dynamic symbol lookup
> 	for symbols with non-default visibility.

I guess this is ok.


r~

Re: RFC: Fix ia64 visibility

[H. J. Lu]

On Thu, May 08, 2003 at 01:01:43AM -0700, Richard Henderson wrote:
> On Wed, May 07, 2003 at 10:51:53PM -0700, H. J. Lu wrote:
> > I didn't create a new function since it isn't used anywhere else.
> 
> Yet.

We will add a new function when it is needed.

> 
> > 	* elfxx-ia64.c (_bfd_elf_link_hash_hide_symbol): Also clear the
> > 	want_plt field.
> > 	(elfNN_ia64_relocate_section): Don't do dynamic symbol lookup
> > 	for symbols with non-default visibility.
> 
> I guess this is ok.
> 

Done. BTW, there are no regressions with "make check" on glibc and
gcc from CVS.


H.J.