Bug 3041 - Bogus jump to weak symbol on m68k-unknown-netbsd
Summary: Bogus jump to weak symbol on m68k-unknown-netbsd
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gas (show other bugs)
Version: 2.18
: P2 normal
Target Milestone: ---
Assignee: unassigned
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-08-14 12:57 UTC by Vincent Rivière
Modified: 2011-02-07 00:04 UTC (History)
1 user (show)

See Also:
Host: i686-pc-linux-gnu
Target: m68k-unknown-netbsd
Build: i686-pc-linux-gnu
Last reconfirmed:


Attachments
Do not convert fixups against weak symbols (460 bytes, patch)
2006-09-07 16:47 UTC, Nick Clifton
Details | Diff
When writing out a.out relocations, treat relocs against weak symbols as if they were against externals (449 bytes, patch)
2006-09-09 08:52 UTC, Nick Clifton
Details | Diff
Write correct values and relocs into a.out object files (846 bytes, patch)
2007-04-28 21:38 UTC, Vincent Rivière
Details | Diff
Write correct offsets into a.out object files (526 bytes, patch)
2007-05-08 14:35 UTC, Vincent Rivière
Details | Diff
Fix addend for weak references to data symbols (1.80 KB, patch)
2009-10-03 16:38 UTC, Vincent Rivière
Details | Diff
Patch for pc-relative weak references (850 bytes, patch)
2010-11-21 19:36 UTC, Vincent Rivière
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vincent Rivière 2006-08-14 12:57:02 UTC
When assembled, a jump to weak symbol produces a jump to an invalid address. I 
found that problem on m68k-unknown-netbsd, but it may affect all m68k-*-aout 
targets.

$ cat bug.s
        jmp     mylabel
        nop
        .weak mylabel
mylabel:
        nop

$ as bug.s -o bug.o
$ objdump -d bug.o

bug.o:     file format a.out-m68k-netbsd

Disassembly of section .text:

00000000 <mylabel-0x6>:
   0:   4efa 000a       jmp %pc@(c <mylabel+0x6>)
   4:   4e71            nop

00000006 <mylabel>:
   6:   4e71            nop

The value of the jmp instruction is wrong : it should be %pc@(6 <mylabel>)
It might not be a problem, because there is a relocation.

$ objdump -r bug.o

bug.o:     file format a.out-m68k-netbsd

RELOCATION RECORDS FOR [.text]:
OFFSET   TYPE              VALUE
00000002 DISP16            .text

Here, it is clearly wrong. The relocation offset is good, but the value should 
be "mylabel" instead of ".text"
After linking, there is no more relocation, but the value of the jump is still 
bad.

The same problem was present in gas 2.14 for m68k-unknown-netbsd (already using 
BFD).
The problem was not present in gas 2.14 for m68k-linux-aout (not using BFD) :

$ objdump -d bug.o # binutils 2.14 m68k-linux-aout

bug.o:     file format a.out-zero-big

Disassembly of section .text:

00000000 <mylabel-0x6>:
   0:   4efa 0004       jmp %pc@(6 <mylabel>)
   4:   4e71            nop

00000006 <mylabel>:
   6:   4e71            nop

I think the above result is correct and the current version of gas for m68k-
unknown-netbsd should behave like that.
Comment 1 Nick Clifton 2006-09-07 16:47:51 UTC
Created attachment 1287 [details]
Do not convert fixups against weak symbols
Comment 2 Nick Clifton 2006-09-07 16:48:34 UTC
Hi Vincent,

  Please could you try the uploaded patch.  I think that it fix the assembler
for you.

Cheers
  Nick
Comment 3 Vincent Rivière 2006-09-08 20:46:57 UTC
Sorry, your fix does not work because... it is not compiled !!!
It is inside a #ifdef OBJ_ELF block, but the m68k-unknown-netbsd target uses 
the a.out format.
Furthermore, I think you forgot to remove a double ampersand in your fix : 
S_IS_WEAK (&& fixP->fx_addsy))

Thank you for trying to solve this bug !
Comment 4 Nick Clifton 2006-09-09 08:50:38 UTC
Hi Vincent,

  Boy I must have been asleep when I wrote that patch, it was completely bogus.
 Oh well, I have uploaded another one for you to try.  This time I have actually
tested it with the test case you supplied, so I think that it should work. 
Please let me know how you get on.

Cheers
  Nick
Comment 5 Nick Clifton 2006-09-09 08:52:41 UTC
Created attachment 1292 [details]
When writing out a.out relocations, treat relocs against weak symbols as if they were against externals
Comment 6 Vincent Rivière 2006-09-09 10:31:21 UTC
A lot better this time !
But half-fixed.

$ as bug.s -o bug.o
$ objdump -d bug.o

bug.o:     file format a.out-m68k-netbsd

Disassembly of section .text:

00000000 <mylabel-0x8>:
   0:   4ef9 0000 0010  jmp 10 <mylabel+0x8>
   6:   4e71            nop

00000008 <mylabel>:
   8:   4e71            nop
        ...

We can see a strange value for the jmp (0x10), but it dosn't matter because...

$ objdump -r bug.o

bug.o:     file format a.out-m68k-netbsd

RELOCATION RECORDS FOR [.text]:
OFFSET   TYPE              VALUE
00000002 32                mylabel

... the relocation is good !

Let's go ahead and link it :
$ ld bug.o -o bug
$ objdump -d bug

bug:     file format a.out-m68k-netbsd

Disassembly of section .text:

00002020 <bug.o>:
    2020:       4ef9 0000 2038  jmp 2038 <__etext+0xc>
    2026:       4e71            nop

00002028 <mylabel>:
    2028:       4e71            nop
        ...

0000202c <__etext>:
        ...

Bad : The value of the jump should be 0x2028, but it is 0x2038

$ objdump -r bug

bug:     file format a.out-m68k-netbsd

And there is no relocation.

Note that I obtain the same result when I link bug.o using ld 2.14.
So I thing there is still a problem with gas.

Nice work... just another effort and you'll get it !
Comment 7 Vincent Rivière 2007-04-09 21:59:17 UTC
I found an allusion to a weak-bug in bfd/aout-cris.c
In MY (swap_ext_reloc_out) :
...
      if (bfd_is_und_section (bfd_get_section (sym))
	  /* Remember to check for weak symbols; they count as global.  */
	  || (sym->flags & (BSF_GLOBAL | BSF_WEAK)) != 0)
	r_extern = 1;

I applied that fix into bfd/aoutx.h, but it didn't change anything on the 
testcase described here :(
Comment 8 Vincent Rivière 2007-04-28 21:38:20 UTC
Created attachment 1739 [details]
Write correct values and relocs into a.out object files

After spending *weeks* in the debugger, I finally managed to get it work.
My patch is attached to this message. It is is composed of 4 parts :

1) The patch provided by Nick in Comment #5. It is necessary to generate a
reloc with the correct symbol name.

2) Never relax references to weak symbols, as they must be full absolute
pointers in order to be relocated.

3) In md_apply_fix(): Always put zero values into frags referencing a weak
symbol.

4) In tc_gen_reloc(): Adjust addend in order to force bfd_install_relocation()
to put a zero value into the frags referencing a weak symbol.

This patch affects only a.out and m68k, and only when weak symbols are used.
It may not affect anything else.
I think it works as expected.

Could someone verify it is OK and check this in, please ?

Vincent


Here are the detailed results :

$ cat bug.s
	jmp	mylabel
	nop
	.weak	mylabel
mylabel:
	nop

$ as bug.s -o bug.o
$ objdump -d bug.o

bug.o:	   file format a.out-m68k-netbsd

Disassembly of section .text:

00000000 <mylabel-0x8>:
   0:	4ef9 0000 0000	jmp 0 <mylabel-0x8>
   6:	4e71		nop

00000008 <mylabel>:
   8:	4e71		nop
	...

$ objdump -r bug.o

bug.o:	   file format a.out-m68k-netbsd

RELOCATION RECORDS FOR [.text]:
OFFSET	 TYPE		   VALUE
00000002 32		   mylabel

$ nm bug.o
00000008 W mylabel

$ ld bug.o -o bug
$ objdump -d bug

bug:	 file format a.out-m68k-netbsd

Disassembly of section .text:

00002020 <bug.o>:
    2020:	4ef9 0000 2028	jmp 2028 <mylabel>
    2026:	4e71		nop

00002028 <mylabel>:
    2028:	4e71		nop
	...

0000202c <__etext>:
	...
Comment 9 Nick Clifton 2007-05-03 17:25:06 UTC
Hi Vincent,

  Thanks for the patch.  I have exmained it and it is fine, so I have checked it
into the sources along with these ChangeLog entries.

Cheers
  Nick

gas/ChangeLog
2007-05-03  Vincent Riviere  <vincent.riviere@freesbee.fr>
	    Nick Clifton  <nickc@redhat.com>

	PR gas/3041
	* config/tc-m68k.c (relaxable_symbol): Do not relax weak symbols.
	(tc_gen_reloc): Adjust the addend of relocs against weak symbols.
	(md_apply_fix): Put zero values into the frags referencing weak
	symbols.

bfd/ChangeLog
2007-05-03  Vincent Riviere  <vincent.riviere@freesbee.fr>
	    Nick Clifton  <nickc@redhat.com>

	PR gas/3041
	* aoutx.h (swap_std_reloc_out): Treat relocs against weak symbols
	in the same way as relocs against external symbols.
Comment 10 Vincent Rivière 2007-05-03 20:04:06 UTC
Great !
It works as expected.
Even the linker does its job when linking with another object containing a 
strong symbol with the same name.

Thank you Nick !
Comment 11 Vincent Rivière 2007-05-08 14:35:53 UTC
Created attachment 1778 [details]
Write correct offsets into a.out object files

There is still a bug when the reference to the weak symbol contains an offset.
With my previous patch, the offset is unconditionnaly set to 0.

Here is the updated testcase :

$ cat bug.s
	jmp	mylabel+2
	nop
	.weak	mylabel
mylabel:
	nop
	nop

$ as bug.s -o bug.o
$ objdump -d bug.o

bug.o:	   file format a.out-m68k-netbsd

Disassembly of section .text:

00000000 <mylabel-0x8>:
   0:	4ef9 0000 0000	jmp 0 <mylabel-0x8>
   6:	4e71		nop

00000008 <mylabel>:
   8:	4e71		nop
   a:	4e71		nop

We can see that the "+2" has disappeared.

The attached patch fixes this problem.
Here is the result after applying it :

$ objdump -d bug.o

bug.o:	   file format a.out-m68k-netbsd

Disassembly of section .text:

00000000 <mylabel-0x8>:
   0:	4ef9 0000 0002	jmp 2 <mylabel-0x6>
   6:	4e71		nop

00000008 <mylabel>:
   8:	4e71		nop
   a:	4e71		nop

Now, the right offset is written into the code segment.
The reloc and linker output are good, too.

I think this time it should be correct.

When searching for md_apply_fix() in BFD sources, I found various workarounds
against bfd_install_relocation(). ELF targets seems to be OK, but every other
target uses its own workaround... I'm sure that there are still hidden bugs
with weak symbols.

Could someone check this patch, please ?

Vincent
Comment 12 Vincent Rivière 2007-05-08 14:37:41 UTC
Reopen the bug due to the previous comment.
Comment 13 Nick Clifton 2007-05-15 10:21:32 UTC
Hi Vincent,

  Thanks for the revised patch.  I have applied it, and added your revised test
case as an entry in the m68k gas testsuite.

Cheers
  Nick

gas/ChangeLog
2007-05-15  Vincent Riviere  <vincent.riviere@freesbee.fr>	

	PR gas/3041
	* config/tc-m68k.c (relaxable_symbol): Make sure that the correct
	addend is stored for relocs against weak symbols.
	(md_apply_fix): So not loose track of addend for relocs against
	weak symbols.

gas/testsuite/ChangeLog
2007-05-15  Vincent Riviere  <vincent.riviere@freesbee.fr>
	    Nick Clifton  <nickc@redhat.com>

	PR gas/3041
	* gas/m68k/p3041.s: New test case.
	* gas/m68k/p3041.d: New expected disassembly.
	* gas/m68k/all.exp: Run new test for m68k-*-netbsd toolchains.
	Only run arch-cpu-1 test for ELF based toolchains.
Comment 14 Vincent Rivière 2007-05-15 21:44:09 UTC
Great !
Thank you, Nick.
My testcase passes.

But some tests fails :

1) gas/all/weakref1u.d
It seems that this test should not be run on a.out targets.
I propose to not-target it for m68k-*-netbsd.

2) gas/m68k/br-isa[abc].d
They might have been broken by my "addend" fix, in case of relative relocations.
Comment 15 Vincent Rivière 2007-05-17 16:34:53 UTC
After looking at this a bit closer, I think these 4 failures are unrelated to 
my patch. Furthermore, I think the actual result is good - atleast for m68k-*-
netbsd. But it should be checked by a m68k and a.out guru.

Vincent
Comment 16 Vincent Rivière 2009-10-03 16:31:29 UTC
The resulting addend is still wrong when the referenced symbol is defined as 
weak in the .data or .bss section.
This happens in binutils 2.19.1 and the latest 2.20 from CVS.
Comment 17 Vincent Rivière 2009-10-03 16:38:24 UTC
Created attachment 4250 [details]
Fix addend for weak references to data symbols

The attached patch fixes this problem.
I included a detailed comment about the workaround, and I updated the
testcases.
This patch only affects weak symbols on m68k/a.out targets.
Please commit it into HEAD and the 2.20 branch, and in the 2.19 branch if it is
still opened.
Comment 18 Vincent Rivière 2009-10-07 20:11:47 UTC
Here are proposals for the ChangeLog.

gas:
2009-10-07  Vincent Riviere  <vincent.riviere@freesbee.fr>	

	PR gas/3041
	* config/tc-m68k.c (tc_gen_reloc): Fix addend for relocations
	located in data section an referencing a weak symbol.

gas/testsuite:

2009-10-07  Vincent Riviere  <vincent.riviere@freesbee.fr>	

	PR gas/3041
	* gas/m68k/all.exp: Added "p3041data".
	* gas/m68k/p3041.d, gas/m68k/p3041.s: Added tests of weak references
	from text section to all possible sections.
	* gas/m68k/p3041data.d, gas/m68k/p3041data.s: New test. Check weak
	references from data section.
Comment 19 cvs-commit@gcc.gnu.org 2009-10-13 08:55:49 UTC
Subject: Bug 3041

CVSROOT:	/cvs/src
Module name:	src
Changes by:	nickc@sourceware.org	2009-10-13 08:55:31

Modified files:
	gas            : ChangeLog 
	gas/config     : tc-m68k.c 
	gas/testsuite  : ChangeLog 
	gas/testsuite/gas/m68k: all.exp p3041.d p3041.s 
Added files:
	gas/testsuite/gas/m68k: p3041data.d p3041data.s 

Log message:
	gas:
	2009-10-07  Vincent Riviere  <vincent.riviere@freesbee.fr>
	
	PR gas/3041
	* config/tc-m68k.c (tc_gen_reloc): Fix addend for relocations
	located in data section an referencing a weak symbol.
	
	gas/testsuite:
	
	2009-10-07  Vincent Riviere  <vincent.riviere@freesbee.fr>
	
	PR gas/3041
	* gas/m68k/all.exp: Added "p3041data".
	* gas/m68k/p3041.d, gas/m68k/p3041.s: Added tests of weak references
	from text section to all possible sections.
	* gas/m68k/p3041data.d, gas/m68k/p3041data.s: New test. Check weak
	references from data section.

Patches:
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/ChangeLog.diff?cvsroot=src&r1=1.3982&r2=1.3983
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/config/tc-m68k.c.diff?cvsroot=src&r1=1.111&r2=1.112
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/testsuite/ChangeLog.diff?cvsroot=src&r1=1.1566&r2=1.1567
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041data.d.diff?cvsroot=src&r1=NONE&r2=1.1
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041data.s.diff?cvsroot=src&r1=NONE&r2=1.1
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/all.exp.diff?cvsroot=src&r1=1.19&r2=1.20
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041.d.diff?cvsroot=src&r1=1.1&r2=1.2
http://sources.redhat.com/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041.s.diff?cvsroot=src&r1=1.1&r2=1.2

Comment 20 Nick Clifton 2009-10-13 08:55:58 UTC
Hi Vincent,

  Thanks for the patches.  I have applied them, along with your changelog entries.

Cheers
  Nick
Comment 21 Vincent Rivière 2009-10-13 19:41:00 UTC
Many thanks, Nick.
I see it is too late for 2.20.
Comment 22 Alan Modra 2009-10-13 23:47:01 UTC
This patch probably is relevant for other targets too.  I can't ask you to look
at them as I fully sympathize with your comment #8 regarding time spent in the
debugger.
Comment 23 Vincent Rivière 2009-10-13 23:54:09 UTC
Yes, I believe other targets using a.out for other processors are affected, 
too, but I cowardly fixed the bug only for m68k.
It would probably be more clean to fix bfd_install_relocation(), but that 
would affect all the targets and their own workarounds.
Comment 24 Vincent Rivière 2010-11-21 19:36:15 UTC
Created attachment 5133 [details]
Patch for pc-relative weak references

In reference to this commit:
http://sourceware.org/ml/binutils/2010-09/msg00122.html

I confirm there was an additional bug, and that patch fixed it.
I have attached a new testcase for checking that.
I have also enabled the p3041 testcase for all the m68k-aout targets.

Please commit it into the 2.21 branch and the trunk, with the following changelog entry:

gas/testsuite:
	
2010-10-XX  Vincent Riviere  <vincent.riviere@freesbee.fr>
	
PR gas/3041
* gas/m68k/all.exp: Added "p3041pcrel" and enabled the p3041 tests for all the m68k-aout targets.
* gas/m68k/p3041pcrel.d, gas/m68k/p3041pcrel.s: New test. Check pc-relative weak
references.
Comment 25 cvs-commit@gcc.gnu.org 2011-02-07 00:04:12 UTC
CVSROOT:	/cvs/src
Module name:	src
Changes by:	amodra@sourceware.org	2011-02-07 00:04:09

Modified files:
	gas/testsuite  : ChangeLog 
	gas/testsuite/gas/m68k: all.exp 
Added files:
	gas/testsuite/gas/m68k: p3041pcrel.d p3041pcrel.s 

Log message:
	PR gas/3041
	* gas/m68k/p3041pcrel.s, * gas/m68k/p3041pcrel.d: New test.
	* gas/m68k/all.exp: Add "p3041pcrel" and enable p3041 tests for
	all m68k-aout targets.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/ChangeLog.diff?cvsroot=src&r1=1.1844&r2=1.1845
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041pcrel.d.diff?cvsroot=src&r1=NONE&r2=1.1
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041pcrel.s.diff?cvsroot=src&r1=NONE&r2=1.1
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/all.exp.diff?cvsroot=src&r1=1.25&r2=1.26
Comment 26 cvs-commit@gcc.gnu.org 2011-02-07 00:04:47 UTC
CVSROOT:	/cvs/src
Module name:	src
Branch: 	binutils-2_21-branch
Changes by:	amodra@sourceware.org	2011-02-07 00:04:44

Modified files:
	gas/testsuite  : ChangeLog 
	gas/testsuite/gas/m68k: all.exp 
Added files:
	gas/testsuite/gas/m68k: p3041pcrel.d p3041pcrel.s 

Log message:
	PR gas/3041
	* gas/m68k/p3041pcrel.s, * gas/m68k/p3041pcrel.d: New test.
	* gas/m68k/all.exp: Add "p3041pcrel" and enable p3041 tests for
	all m68k-aout targets.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/ChangeLog.diff?cvsroot=src&only_with_tag=binutils-2_21-branch&r1=1.1802.2.4&r2=1.1802.2.5
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041pcrel.d.diff?cvsroot=src&only_with_tag=binutils-2_21-branch&r1=NONE&r2=1.1.2.1
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/p3041pcrel.s.diff?cvsroot=src&only_with_tag=binutils-2_21-branch&r1=NONE&r2=1.1.2.1
http://sourceware.org/cgi-bin/cvsweb.cgi/src/gas/testsuite/gas/m68k/all.exp.diff?cvsroot=src&only_with_tag=binutils-2_21-branch&r1=1.24&r2=1.24.2.1