[PATCH, 2.20] Fix bug in PE ld: DLL base relocations vs. weak symbols.

[Dave Korn <dave dot korn dot cygwin at googlemail dot com>]

    Hi all,

  Now that Cygwin is using weak symbols in anger for the first time, a problem
showed up in ld(*), which led to DLLs with references to weak externals not
being correctly rebased when they can't load at their normal ImageBase.

  The problem turns out to be the equal and opposite counterpart to an earlier
one patched by Danny some time ago(**).  In that bug, base relocs were
generated against references to undefined weak symbols, causing them no longer
to have the expected NULL value that they should have had at load time to show
they had not been resolved when the DLL was rebased during loading.

  In this bug, the first problem is that no base relocation was generated for
a weak symbol even though it had been resolved against a strong definition.
That was because the test in pe-dll.c/generate_reloc() wasn't sensitive to the
--wrap options, and thought the symbol was undefined, when it in fact got
found by wrapper resolution.  Fixed by calling the wrapped hash lookup routine
instead of the plain one.

  The second problem is that the way weak externals work on PE is by providing
a default value (in an associated auxiliary symbol) for use if no strong
definition is found at final link time.  We want to generate a base reloc even
if there isn't a strong definition, just so long as the default value is
pointing at a real location inside the DLL, because that's where the any
(ordinary bfd non-base-) relocs will get resolved to.  We only don't want to
generate a base reloc against a weak symbol if it's not only undefined, but
the default symbol is also undefined (indicated in practice by pointing to
zero in the *ABS* section).  So this required looking up the symbol's
auxiliary symbol(***) to check what kind of default, if any, was supplied.

ld/ChangeLog:

	* pe-dll.c (generate_reloc): Take account of wrapper options when
	testing if a weak symbol is defined or not, and when it is not,
	consider whether the default value requires a base reloc anyway.

  Built and tested on i686-pc-cygwin with no regressions, and it solves the
testcase that first showed the bug by generating a rebaseable cygstdc++-6.dll.
 I'm also going to spin a gcc release with the patched binutils and give that
some testing before I check it in, which will give anyone interested a chance
to comment.

  This is a fairly serious bug; address space collisions can and do happen, so
DLLs have to be rebaseable.  OK to backport to the release branch?

    cheers,
      DaveK
-- 
(*)   -
http://sourceforge.net/mailarchive/forum.php?thread_name=4AE00A8C.6040808%40users.sourceforge.net&forum_name=cygwin-ports-general
or http://tinyurl.com/yhsu34f if that gets wrapped.
(**)  - http://sourceware.org/ml/binutils/2008-07/msg00372.html
(***) - Note how this continues the slightly ugly tradition in the PE ports of
LD of diving right into libcoff.h in order to look at format-specific bfd
backend data.  I'll be tackling that as a separate matter, and since it's not
critical to this fix, I reckoned this wasn't making the situation any
significantly worse.  What I think this really needs is a proper cleanup, with
moving a whole bunch of functionality down into coff-targeted bfd where it can
be shared by both ld and dlltool, eliminating a whole lot of duplicated code
between them (and hopefully a bunch of bugs and inconsistencies with it); in
the course of doing that we'd want to define a proper api for getting at all
this information, and not have to have ld use bfd internal headers.

Index: ld/pe-dll.c
===================================================================
RCS file: /cvs/src/src/ld/pe-dll.c,v
retrieving revision 1.124
diff -p -u -r1.124 pe-dll.c
--- ld/pe-dll.c	27 Nov 2009 09:00:36 -0000	1.124
+++ ld/pe-dll.c	7 Dec 2009 08:11:07 -0000
@@ -1361,10 +1361,29 @@ generate_reloc (bfd *abfd, struct bfd_li
 		  if (sym->flags == BSF_WEAK)
 		    {
 		      struct bfd_link_hash_entry *blhe
-			= bfd_link_hash_lookup (info->hash, sym->name,
+			= bfd_wrapped_link_hash_lookup (abfd, info, sym->name,
 						FALSE, FALSE, FALSE);
-		      if (!blhe || blhe->type != bfd_link_hash_defined)
-			continue;		      
+		      if (blhe && blhe->type == bfd_link_hash_undefweak)
+			{
+			  /* Check aux sym and see if it is defined or not. */
+			  struct coff_link_hash_entry *h, *h2;
+			  h = (struct coff_link_hash_entry *)blhe;
+			  if (h->symbol_class != C_NT_WEAK || h->numaux != 1)
+			    continue;
+			  h2 = h->auxbfd->tdata.coff_obj_data->sym_hashes
+						[h->aux->x_sym.x_tagndx.l];
+			  /* We don't want a base reloc if the aux sym is not
+			     found, undefined, or if it is the constant ABS
+			     zero default value.  (We broaden that slightly by
+			     not testing the value, just the section; there's
+			     no reason we'd want a reference to any absolute
+			     address to get relocated during rebasing).  */
+			  if (!h2 || h2->root.type == bfd_link_hash_undefined
+				|| h2->root.u.def.section == &bfd_abs_section)
+			    continue;
+			}
+		      else if (!blhe || blhe->type != bfd_link_hash_defined)
+			continue;
 		    }
 
 		  sym_vma = (relocs[i]->addend