PATCH: PR ld/14323: Linker fails to handle weak alias with __start_SECNAME symbol

PATCH: PR ld/14323: Linker fails to handle weak alias with __start_SECNAME symbol

[H.J. Lu]

Hi,

When we check symbol alias in a dynamic object, we should also check
symbol size.  The __start_SECNAME symbol created by linker may have
the same section and value, but its size is 0.  OK to install?

Thanks.

H.J.
---
bfd/

2012-07-02  H.J. Lu  <hongjiu.lu@intel.com>

	PR ld/14323
	* elflink.c (elf_sort_symbol): Also check symbol size.
	(elf_link_add_object_symbols): Also check symbol size for
	symbol alias in a dynamic object.

ld/testsuite/

2012-07-02  H.J. Lu  <hongjiu.lu@intel.com>

	PR ld/14323
	* ld-elf/pr14323-1.c: New.
	* ld-elf/pr14323-2.c: Likewise.

	* ld-elf/shared.exp (build_tests): Add libpr14323-2.so.
	(run_tests): Add pr14323.

diff --git a/bfd/elflink.c b/bfd/elflink.c
index d9e1abe..3e812e1 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -3126,7 +3126,7 @@ on_needed_list (const char *soname, struct bfd_link_needed_list *needed)
   return FALSE;
 }
 
-/* Sort symbol by value and section.  */
+/* Sort symbol by value, section and size.  */
 static int
 elf_sort_symbol (const void *arg1, const void *arg2)
 {
@@ -3145,7 +3145,8 @@ elf_sort_symbol (const void *arg1, const void *arg2)
       if (sdiff != 0)
 	return sdiff > 0 ? 1 : -1;
     }
-  return 0;
+  vdiff = h1->size - h2->size;
+  return vdiff == 0 ? 0 : vdiff > 0 ? 1 : -1;
 }
 
 /* This function is used to adjust offsets into .dynstr for
@@ -4690,6 +4691,7 @@ error_free_dyn:
 	  struct elf_link_hash_entry *hlook;
 	  asection *slook;
 	  bfd_vma vlook;
+	  bfd_size_type sizelook;
 	  long ilook;
 	  size_t i, j, idx;
 
@@ -4703,6 +4705,7 @@ error_free_dyn:
 		      || hlook->root.type == bfd_link_hash_indirect);
 	  slook = hlook->root.u.def.section;
 	  vlook = hlook->root.u.def.value;
+	  sizelook = hlook->size;
 
 	  ilook = -1;
 	  i = 0;
@@ -4726,8 +4729,16 @@ error_free_dyn:
 		    i = idx + 1;
 		  else
 		    {
-		      ilook = idx;
-		      break;
+		      vdiff = sizelook - h->size;
+		      if (vdiff < 0)
+			j = idx;
+		      else if (vdiff > 0)
+			i = idx + 1;
+		      else
+			{
+			  ilook = idx;
+			  break;
+			}
 		    }
 		}
 	    }
diff --git a/ld/testsuite/ld-elf/pr14323-1.c b/ld/testsuite/ld-elf/pr14323-1.c
new file mode 100644
index 0000000..9f63f4c
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr14323-1.c
@@ -0,0 +1,17 @@
+#include <stdlib.h>
+#include <stdio.h>
+
+extern int foo_alias;
+extern char *bar ();
+
+int
+main ()
+{
+  if (foo_alias != 0)
+    abort ();
+  bar ();
+  if (foo_alias != -1)
+    abort ();
+  printf ("PASS\n");
+  return 0;
+}
diff --git a/ld/testsuite/ld-elf/pr14323-2.c b/ld/testsuite/ld-elf/pr14323-2.c
new file mode 100644
index 0000000..34753d1
--- /dev/null
+++ b/ld/testsuite/ld-elf/pr14323-2.c
@@ -0,0 +1,13 @@
+int foo __attribute__ ((section ("_data_foo"))) = 0;
+extern int foo_alias __attribute__ ((weak, alias ("foo")));
+extern char __start__data_foo;
+asm (".type __start__data_foo,%object");
+int x1 = 1;
+int x2 = 2;
+
+char *
+bar ()
+{
+  foo = -1;
+  return &__start__data_foo;
+}
diff --git a/ld/testsuite/ld-elf/shared.exp b/ld/testsuite/ld-elf/shared.exp
index aaaa85b..c00f3e5 100644
--- a/ld/testsuite/ld-elf/shared.exp
+++ b/ld/testsuite/ld-elf/shared.exp
@@ -170,6 +170,9 @@ set build_tests {
   {"Build libpr13250-3.o"
    "-r -nostdlib" ""
    {pr13250-3.c} {} "libpr13250-3.o"}
+  {"Build libpr14323-2.so"
+   "-shared" "-fPIC"
+   {pr14323-2.c} {} "libpr14323-2.so"}
 }
 
 run_cc_link_tests $build_tests
@@ -302,6 +305,9 @@ set run_tests {
     {"Run with pr13250-3.c, libpr13250-1.so and libpr13250-2.so"
      "--as-needed tmpdir/pr13250-3.o tmpdir/libpr13250-1.so tmpdir/libpr13250-2.so" ""
      {dummy.c} "pr13250" "pass.out"}
+    {"Run with pr14323-1.c pr14323-2.so"
+     "tmpdir/libpr14323-2.so" ""
+     {pr14323-1.c} "pr14323" "pass.out"}
 }
 
 # NetBSD ELF systems do not currently support the .*_array sections.

Re: PATCH: PR ld/14323: Linker fails to handle weak alias with __start_SECNAME symbol

[Alan Modra]

On Mon, Jul 02, 2012 at 02:16:57PM -0700, H.J. Lu wrote:
> When we check symbol alias in a dynamic object, we should also check
> symbol size.

Always?  How does this play with cases where _bfd_elf_merge_symbol
returns size_change_ok.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PATCH: PR ld/14323: Linker fails to handle weak alias with __start_SECNAME symbol

[H.J. Lu]

On Tue, Jul 3, 2012 at 5:46 PM, Alan Modra <amodra@gmail.com> wrote:
> On Mon, Jul 02, 2012 at 02:16:57PM -0700, H.J. Lu wrote:
>> When we check symbol alias in a dynamic object, we should also check
>> symbol size.
>
> Always?  How does this play with cases where _bfd_elf_merge_symbol
> returns size_change_ok.
>

Here we are matching a symbol to a weak alias.  Their
sizes must match.

-- 
H.J.

Re: PATCH: PR ld/14323: Linker fails to handle weak alias with __start_SECNAME symbol

[Alan Modra]

On Wed, Jul 04, 2012 at 06:46:59AM -0700, H.J. Lu wrote:
> On Tue, Jul 3, 2012 at 5:46 PM, Alan Modra <amodra@gmail.com> wrote:
> > On Mon, Jul 02, 2012 at 02:16:57PM -0700, H.J. Lu wrote:
> >> When we check symbol alias in a dynamic object, we should also check
> >> symbol size.
> >
> > Always?  How does this play with cases where _bfd_elf_merge_symbol
> > returns size_change_ok.
> 
> Here we are matching a symbol to a weak alias.  Their
> sizes must match.

Two problems.  The first one is that your testsuite addition doesn't
fail for me on x64_64-linux without your elflink.c change.  So the
testcase isn't very useful.  Binary search for a given key when
multiple entries match the key gives you one of the matching entries
but which one depends on exactly how the binary search went, ie. it
depends on other entries in the array.  So when I linked your testcase
I happened to get foo_alias matching foo rather than the failure mode
of foo_alias matching __start__data_foo.

Second problem is that I'm uncertain you can claim "sizes must
match".  If they do match, fine, go with the match.  However if no
size matches exactly, I think it would be better to match the largest
size symbol at the given address.  That way you at least get a match,
and the largest .dynbss/copy reloc.  Maybe I'm fussing a little here.
Aliases generated by gcc with "__attribute__ ((weak, alias (...)))"
require the target be defined in the same compilation unit, so they
will be the same size.  What about __attribute__ ((weakref (..)))?
I don't want to break some existing code that has only two symbols at
one address, a weak and a global, but their sizes don't match.  HJ,
have you built glibc with your patch?

For those a little mystified by what is going on here, we have a
shared library with aliases.  This from the testcase in the PR.

     8: 0000000000201038     0 NOTYPE  GLOBAL DEFAULT   23 __start__data_foo
     9: 0000000000201038     4 OBJECT  WEAK   DEFAULT   23 foo_alias
    10: 0000000000201038     4 OBJECT  GLOBAL DEFAULT   23 foo

Note the differences in sizes (3rd col).  Main program accesses
foo_alias, and the linker is left with a choice of two possible
matching globals for the weak symbol.  Furthermore, the main program
is non-PIC so wants to set up it's own foo_alias in .dynbss.  This
means the size matters, so the choice of global matters.  Choosing the
largest or choosing matching size gets the right one in this case.

There is another twist too.  HJ's testcase also accesses
__start__data_foo in main, and it too is allocated in .dynbss, except
that since its size was zero no space is actually allocated.  So
__start__data_foo in main has the same address as foo/foo_alias only
if the symbols are processed in exactly the right order and no other
.dynbss symbols intervene.  Yes, aliases and .dynbss are horrible.  We
don't pretend to handle this correctly.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PATCH: PR ld/14323: Linker fails to handle weak alias with __start_SECNAME symbol

[H.J. Lu]

On Sat, Jul 7, 2012 at 6:24 AM, Alan Modra <amodra@gmail.com> wrote:
> On Wed, Jul 04, 2012 at 06:46:59AM -0700, H.J. Lu wrote:
>> On Tue, Jul 3, 2012 at 5:46 PM, Alan Modra <amodra@gmail.com> wrote:
>> > On Mon, Jul 02, 2012 at 02:16:57PM -0700, H.J. Lu wrote:
>> >> When we check symbol alias in a dynamic object, we should also check
>> >> symbol size.
>> >
>> > Always?  How does this play with cases where _bfd_elf_merge_symbol
>> > returns size_change_ok.
>>
>> Here we are matching a symbol to a weak alias.  Their
>> sizes must match.
>
> Two problems.  The first one is that your testsuite addition doesn't
> fail for me on x64_64-linux without your elflink.c change.  So the
> testcase isn't very useful.  Binary search for a given key when
> multiple entries match the key gives you one of the matching entries
> but which one depends on exactly how the binary search went, ie. it
> depends on other entries in the array.  So when I linked your testcase
> I happened to get foo_alias matching foo rather than the failure mode
> of foo_alias matching __start__data_foo.

That is true:

int x1 = 1;
int x2 = 2;

is added to pr14323-2.c to make it fail for me.  I am using GCC 4.7
if it matters.

> Second problem is that I'm uncertain you can claim "sizes must
> match".  If they do match, fine, go with the match.  However if no
> size matches exactly, I think it would be better to match the largest
> size symbol at the given address.  That way you at least get a match,
> and the largest .dynbss/copy reloc.  Maybe I'm fussing a little here.
> Aliases generated by gcc with "__attribute__ ((weak, alias (...)))"
> require the target be defined in the same compilation unit, so they
> will be the same size.  What about __attribute__ ((weakref (..)))?

I don't think it should make a difference since weakref alias isn't
in symbol table.

> I don't want to break some existing code that has only two symbols at
> one address, a weak and a global, but their sizes don't match.  HJ,
> have you built glibc with your patch?

Yes, I used it to build GCC, glibc and Linux kernel. All work fine.

-- 
H.J.

Re: PATCH: PR ld/14323: Linker fails to handle weak alias with __start_SECNAME symbol

[Alan Modra]

Since a binary search may return a match on any of a series of
matching entries, I think we've always had a problem matching up
symbol aliases.  Given a weak and global alias, with the weak symbol
already strongly defined, we'd get both symbols in sorted_sym_hash.
Then fail to find the global if it was first in the qsort output and
the binary search happened to land on the second of the two symbols.

This patch cures that problem and pr14323 too.  I've made a few other
tweaks that might make the code a little faster.

HJ, please commit the modified version of your testcase that fails
without this patch.

	PR ld/14323
	* elflink.c (elf_sort_symbol): Sort by size too.
	(elf_link_add_object_symbols <weakdefs>): Simplify binary search.
	Do not depend on ordering of symbol aliases.  Match largest size.

Index: bfd/elflink.c
===================================================================
RCS file: /cvs/src/src/bfd/elflink.c,v
retrieving revision 1.449
diff -u -p -r1.449 elflink.c
--- bfd/elflink.c	3 Jul 2012 14:44:34 -0000	1.449
+++ bfd/elflink.c	9 Jul 2012 07:21:45 -0000
@@ -3145,7 +3145,7 @@ on_needed_list (const char *soname, stru
   return FALSE;
 }
 
-/* Sort symbol by value and section.  */
+/* Sort symbol by value, section, and size.  */
 static int
 elf_sort_symbol (const void *arg1, const void *arg2)
 {
@@ -3164,7 +3164,8 @@ elf_sort_symbol (const void *arg1, const
       if (sdiff != 0)
 	return sdiff > 0 ? 1 : -1;
     }
-  return 0;
+  vdiff = h1->size - h2->size;
+  return vdiff == 0 ? 0 : vdiff > 0 ? 1 : -1;
 }
 
 /* This function is used to adjust offsets into .dynstr for
@@ -4726,7 +4727,6 @@ error_free_dyn:
 	  struct elf_link_hash_entry *hlook;
 	  asection *slook;
 	  bfd_vma vlook;
-	  long ilook;
 	  size_t i, j, idx;
 
 	  hlook = weaks;
@@ -4740,14 +4740,13 @@ error_free_dyn:
 	  slook = hlook->root.u.def.section;
 	  vlook = hlook->root.u.def.value;
 
-	  ilook = -1;
 	  i = 0;
 	  j = sym_count;
-	  while (i < j)
+	  while (i != j)
 	    {
 	      bfd_signed_vma vdiff;
 	      idx = (i + j) / 2;
-	      h = sorted_sym_hash [idx];
+	      h = sorted_sym_hash[idx];
 	      vdiff = vlook - h->root.u.def.value;
 	      if (vdiff < 0)
 		j = idx;
@@ -4761,24 +4760,36 @@ error_free_dyn:
 		  else if (sdiff > 0)
 		    i = idx + 1;
 		  else
-		    {
-		      ilook = idx;
-		      break;
-		    }
+		    break;
 		}
 	    }
 
 	  /* We didn't find a value/section match.  */
-	  if (ilook == -1)
+	  if (i == j)
 	    continue;
 
-	  for (i = ilook; i < sym_count; i++)
+	  /* With multiple aliases, or when the weak symbol is already
+	     strongly defined, we have multiple matching symbols and
+	     the binary search above may land on any of them.  Step
+	     one past the matching symbol(s).  */
+	  while (++idx != j)
+	    {
+	      h = sorted_sym_hash[idx];
+	      if (h->root.u.def.section != slook
+		  || h->root.u.def.value != vlook)
+		break;
+	    }
+
+	  /* Now look back over the aliases.  Since we sorted by size
+	     as well as value and section, we'll choose the one with
+	     the largest size.  */
+	  while (idx-- != i)
 	    {
-	      h = sorted_sym_hash [i];
+	      h = sorted_sym_hash[idx];
 
 	      /* Stop if value or section doesn't match.  */
-	      if (h->root.u.def.value != vlook
-		  || h->root.u.def.section != slook)
+	      if (h->root.u.def.section != slook
+		  || h->root.u.def.value != vlook)
 		break;
 	      else if (h != hlook)
 		{

-- 
Alan Modra
Australia Development Lab, IBM