[PATCH] PR977: Weak symbols on win32 - take 3.

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

[ See reference links in .sig at end of post. ]

    Hi everyone,

  As discussed earlier, PR977 shows the ld-scripts/weak.exp test failing on
Cygwin.  It also fails on MinGW; the underlying cause is in the generic x86
PE/COFF object format code in GAS.

  My first attempt to fix this ran into the problem of offsets in the addend
fields described by Vincent R. in Bug 3041, comment 11.  During that
discussion, the biggest cause for concern expressed was the need to support
and interoperate with the native toolset and the binaries they generate.
These are clearly valid and important concerns, so I am hoping to get a
consensus behind the changes I am proposing.  This has nothing to do with
ELF-like or C_WEAKEXT symbols, and purely relates to binutils' handling of the
.weak pseudo-op and the use of PE "weak externals" to implement it.

  I have been testing code derived from the failing testcase against
Microsoft's ML.EXE (MASM) and LINK.EXE, in the free-as-in-beer VS Express 2008
versions.  (I also briefly tested the version of ML.EXE that shipped with the
MSVC6 'processor pack', but found its implementation of weak externals to be
too buggy to use; it generates random crud bytes in relocated operand fields).

  I have added attachments to pr977 containing testcase sources, object and
executable binaries generated by both MS and GNU toolchains, and detailed
analyses of the generated symbols and relocation entries in both.  I won't go
into length here, because there's a lot of it, but if I'm correct, then the
PHB-level bullet-point summary is:


 o   We currently generate relocations against weak symbols differently
     from how the MS tools generate them.

 o   The thing that we do differently is the cause of the testcase FAIL.

 o   The MS tools would PASS our test because they do what it is expecting.

 o   What we do differently is to partially resolve the reference in the
     assembler, which is unexpected considering that we can't know how the
     symbol should actually be resolved until final link time.

 o   This also appears to be the obvious correct way to handle relocs
     against weak symbols and our way appears to be simply incorrect.

 o   If we patch binutils to generate the same kind of relocs, the resulting
     binaries are interoperable with MS tools and objects and the test PASSes.


  So, I suggest applying the attached patch.  It fixes the testsuite FAIL for
both MinGW and Cygwin, and as far as I've been able to determine it is both
more correct and more interoperable with the native tools than the current
situation.

  I now need some help from the MinGW side: Danny and Aaron, can you possibly
test the attached patch against whatever use cases you have for the PE weak
externals extension, and give at least a quick eyeball or two to the documents
attached to the PR?  I really don't want to break anything, but on the other
hand I'm not sure how (or even if) it's managing to work now!


gas/ChangeLog:

	* config/tc-i386.c (md_estimate_size_before_relax):  Don't relax
	branches to weak symbols.
	(md_apply_fix):  Don't convert fixes against weak symbols to
	section-relative offsets, but save addend for later reloc emission.
	(tc_gen_reloc):  When emitting reloc against weak symbol, adjust
	addend to pre-compensate for bfd_install_relocation.


  Is this OK by _everyone_ concerned?

    cheers,
      DaveK
-- 
http://sourceware.org/bugzilla/show_bug.cgi?id=977
ld test weak fails on cygwin
[ http://sourceware.org/bugzilla/attachment.cgi?id=3962&action=view
  http://sourceware.org/bugzilla/attachment.cgi?id=3963&action=view
  http://sourceware.org/bugzilla/attachment.cgi?id=3973&action=view
  http://sourceware.org/bugzilla/attachment.cgi?id=3974&action=view ]

http://sourceware.org/bugzilla/show_bug.cgi?id=3041
Bogus jump to weak symbol on m68k-unknown-netbsd
[ http://sourceware.org/bugzilla/show_bug.cgi?id=3041#c11 ]

http://sourceware.org/ml/binutils/2009-03/threads.html#00314
[PATCH/RFC] Fix LD test FAIL: weak symbols on Cygwin

Index: gas/config/tc-i386.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-i386.c,v
retrieving revision 1.375
diff -p -u -r1.375 tc-i386.c
--- gas/config/tc-i386.c	22 May 2009 15:57:23 -0000	1.375
+++ gas/config/tc-i386.c	29 May 2009 03:57:40 -0000
@@ -6956,6 +6956,10 @@ md_estimate_size_before_relax (fragP, se
 	  && (S_IS_EXTERNAL (fragP->fr_symbol)
 	      || S_IS_WEAK (fragP->fr_symbol)))
 #endif
+#if defined (OBJ_COFF) && defined (TE_PE)
+      || (OUTPUT_FLAVOR == bfd_target_coff_flavour 
+	  && S_IS_WEAK (fragP->fr_symbol))
+#endif
       )
     {
       /* Symbol is undefined in this segment, or we need to keep a
@@ -7249,6 +7253,12 @@ md_apply_fix (fixP, valP, seg)
 	value += md_pcrel_from (fixP);
 #endif
     }
+#if defined (OBJ_COFF) && defined (TE_PE)
+  if (fixP->fx_addsy != NULL && S_IS_WEAK (fixP->fx_addsy))
+    {
+      value -= S_GET_VALUE (fixP->fx_addsy);
+    }
+#endif
 
   /* Fix a few things - the dynamic linker expects certain values here,
      and we must not disappoint it.  */
@@ -7312,6 +7322,16 @@ md_apply_fix (fixP, valP, seg)
   /* Are we finished with this relocation now?  */
   if (fixP->fx_addsy == NULL)
     fixP->fx_done = 1;
+#if defined (OBJ_COFF) && defined (TE_PE)
+  else if (fixP->fx_addsy != NULL && S_IS_WEAK (fixP->fx_addsy))
+    {
+      fixP->fx_done = 0;
+      /* Remember value for tc_gen_reloc.  */
+      fixP->fx_addnumber = value;
+      /* Clear out the frag for now.  */
+      value = 0;
+    }
+#endif
   else if (use_rela_relocations)
     {
       fixP->fx_no_overflow = 1;
@@ -8214,7 +8234,11 @@ tc_gen_reloc (section, fixp)
 	 vtable entry to be used in the relocation's section offset.  */
       if (fixp->fx_r_type == BFD_RELOC_VTABLE_ENTRY)
 	rel->address = fixp->fx_offset;
-
+#if defined (OBJ_COFF) && defined (TE_PE)
+      else if (fixp->fx_addsy && S_IS_WEAK (fixp->fx_addsy))
+	rel->addend = fixp->fx_addnumber - (S_GET_VALUE (fixp->fx_addsy) * 2);
+      else
+#endif
       rel->addend = 0;
     }
   /* Use the rela in 64bit mode.  */