[PATCH/RFC] Fix LD test FAIL: weak symbols on Cygwin

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

  This is a pretty hairy one.  I'm not sure if I've taken the correct
approach; I'm not even 100% sure whether to handle it in the linker or the
assembler, but I chose the latter approach to get the ball rolling.

Running /gnu/binutils/src/ld/testsuite/ld-scripts/weak.exp ...
FAIL: weak symbols

  So, anyways, this test currently fails on Cygwin.  What it does is,
assembles two object files, each of which exports two symbols, one weak and
one strong, and each of which references both symbols, then link them
together, into a separate section each with an absolute base, and verify that
the references got correctly resolved to the strong symbols in all cases.

----------------------<weak1.s>----------------------
	.data
	.global	foo1
	.global	sym1
	.weak	sym2
foo1:
	.long	sym1
	.long	sym2
sym1:
	.long	0x12121212
sym2:
	.long	0x34343434
----------------------<weak2.s>----------------------
	.data
	.global	foo2
	.weak	sym1
	.global	sym2
foo2:
	.long	sym1
	.long	sym2
sym1:
	.long	0x56565656
sym2:
	.long	0x78787878
-----------------------------------------------------

  When you read the log files, you see a pattern-match failure checking the
objdump output.  A custom linker script is used to place the .data section
from weak1.o into .text at 0x1000 and the .data section from weak2.o into
.data at 0x2000 in the fully-linked image.  Then the test attempts to match
this objdump output (little-endian):

Contents of section .text:
1000 08100000 0c200000 12121212 34343434
Contents of section .data:
2000 08100000 0c200000 56565656 78787878

... which basically says that both references to sym1 got linked against the
one from weak1.s (0x12121212 at 0x1008) and both references to sym2 got linked
against the one from weak2.s (0x78787878 at 0x200c).


  On Cygwin, it goes badly wrong.  The objdump output we actually get looks
like this:

tmpdir/weak:     file format pei-i386

Contents of section .text:
 1000 08100000 18200000 12121212 34343434  ..... ......4444
Contents of section .data:
 2000 10100000 0c200000 56565656 78787878  ..... ..VVVVxxxx


  What you see there is that the references that were resolved by strong
definitions from within the same object file are correctly relocated (0x1008
at 0x1000 and 0x200c at 0x2004), but the references that were resolved by the
weak symbols at final link time are wrong: 0x2018 instead of 0x200c at 0x1004,
and 0x1010 instead of 0x1008 at 0x2000.  Initially this might look like the
old double-reloc problem, but in fact it's a coincidence that the
displacements look like they've been doubled, owing to the fact that the weak
and strong definitions happen to be at the same relative offsets within their
sections; the resulting relocation is the sum of the two offsets.

  I stepped through gas and ld watching how this came about.  When assembling
the object files, Gas correctly resolves the references for which strong
definitions are present at assembly time, converting the output reloc into a
section-symbol based non-pcrel 32-bit reloc, while the reference for which
only a weak definition is present is emitted as a reloc against the strong
version of the symbol, along with the weak default definition of the symbol:

$ objdump -rts tmpdir/weak1.o

tmpdir/weak1.o:     file format pe-i386

SYMBOL TABLE:
[  0](sec -2)(fl 0x00)(ty   0)(scl 103) (nx 1) 0x00000000 fake
File
[  2](sec  1)(fl 0x00)(ty   0)(scl   3) (nx 1) 0x00000000 .text
AUX scnlen 0x0 nreloc 0 nlnno 0
[  4](sec  2)(fl 0x00)(ty   0)(scl   3) (nx 1) 0x00000000 .data
AUX scnlen 0x10 nreloc 2 nlnno 0
[  6](sec  3)(fl 0x00)(ty   0)(scl   3) (nx 1) 0x00000000 .bss
AUX scnlen 0x0 nreloc 0 nlnno 0
[  8](sec  2)(fl 0x00)(ty   0)(scl   2) (nx 0) 0x00000000 foo1
[  9](sec  2)(fl 0x00)(ty   0)(scl   2) (nx 0) 0x00000008 sym1
[ 10](sec  2)(fl 0x00)(ty   0)(scl   2) (nx 0) 0x0000000c .weak.sym2.foo1
[ 11](sec  0)(fl 0x00)(ty   0)(scl 105) (nx 1) 0x00000000 sym2
AUX lnno 1 size 0x0 tagndx 10


RELOCATION RECORDS FOR [.data]:
OFFSET   TYPE              VALUE
00000000 dir32             .data
00000004 dir32             sym2


Contents of section .data:
 0000 08000000 0c000000 12121212 34343434  ............4444


  However, as you see at 0x4, Gas has put a partial in-place reloc into the
instruction's operand field.  During linking time, BFD comes along and adds
the address of the other (strong) definition of sym2 on top, and so we get a
result of 0x2018 in the output section.

  I considered the possibility that BFD should zero out the in-place field
whenever it finds itself resolving a weak override, but then I decided that on
balance I think Gas is actually doing the wrong thing.  It's deferring
resolving that reference for which only a weak definition is present, so it
should be deferring it entirely, and not half-filling out the in-place field
with what isn't necessarily a meaningful value.  This is just like an
undefined symbol in this context, we know nothing about it's value until final
link-time and we shouldn't be second-guessing what will transpire then.

  So I tried the attached patch.  It works by detecting when a fix is against
a weak symbol in md_apply_fix and refusing to apply it.  Sure enough, it fixes
the testcase, but I'm pretty certain it's wrong.  I think, though, that it's
in the right place (gas rather than ld).  I'm not familiar enough with Gas
internals to know what kinds of expressions I can expect to see in the
fx_addsy member of a fixS at md_apply_fix time.  What I did see is that there
was an O_constant with an X_add_number equal to the offset of the weak
definition of the symbol, and when I zero it out I get zero in the in-place
operand field in the insn and it gets relocated correctly at final link.

  But, I probably haven't done this the right way.  I probably want to
subtract the symbol value from the O_constant rather than blindly setting it
to zero, in case there was some sort of constant offset being applied by e.g.
an addressing mode.  This is where I'd appreciate some advice; there are a lot
of unwritten rules that you have to be careful not to break when you're
changing relocs and fixups, and I'm sure I'm not aware of all of them, just
that you can get things like the pc-relative-double-reloc problem if you're
not careful, or that there might be scoping or visibility issues I'm not aware
of.  A few concise questions:

-  Am I right to try and fix this in Gas rather than BFD?  (IIUC, by the time
it gets to BFD it won't be possible to separate out any constant offset from
the symbol value, which is why I think it has to be done in Gas).
-  Is it ok just to zero out the value of the fx_addsy?
=  Or will I need to do something about the possibility of a constant offset
having been folded into the symbol, and if so how do I get it right?

  I ran the binutils testsuite with the patch applied, and it fixed the
failure and didn't cause any regressions.  I'm now trying a before-and-after
run of the g++ and libstdc++ testsuites, having rebuild libstdc++ and libgcc
from clean with the patched assembler; results won't be in for some hours yet,
but I want to see what it fixes or breaks.


    cheers,
      DaveK

Index: gas/config/tc-i386.c
===================================================================
RCS file: /cvs/src/src/gas/config/tc-i386.c,v
retrieving revision 1.368
diff -p -u -r1.368 tc-i386.c
--- gas/config/tc-i386.c	25 Feb 2009 18:59:52 -0000	1.368
+++ gas/config/tc-i386.c	17 Mar 2009 04:54:49 -0000
@@ -7658,6 +7658,13 @@ md_apply_fix (fixP, valP, seg)
 	value += md_pcrel_from (fixP);
 #endif
     }
+#if defined (OBJ_COFF) && defined (TE_PE)
+  else if (fixP->fx_addsy != NULL && S_IS_WEAK (fixP->fx_addsy))
+    {
+      value = 0;
+      S_SET_VALUE (fixP->fx_addsy, 0);
+    }
+#endif
 
   /* Fix a few things - the dynamic linker expects certain values here,
      and we must not disappoint it.  */
@@ -7721,6 +7728,13 @@ 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;
+      return;
+    }
+#endif
   else if (use_rela_relocations)
     {
       fixP->fx_no_overflow = 1;