Bug 19291 - constant strings still don't always get relocated properly in a relocatable built with gold --script
Summary: constant strings still don't always get relocated properly in a relocatable b...
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: gold (show other bugs)
Version: 2.25
: P2 normal
Target Milestone: ---
Assignee: Cary Coutant
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-25 08:31 UTC by Martin Dorey
Modified: 2017-11-28 01:34 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
the source and binaries, including gold 2.25-built relocatable.o (5.86 KB, application/x-gzip)
2015-11-25 08:31 UTC, Martin Dorey
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Dorey 2015-11-25 08:31:46 UTC
Created attachment 8813 [details]
the source and binaries, including gold 2.25-built relocatable.o

This is a similar test case to Bug 6992, derived from the same code base.  The test case attached to that bug now works for us, with your kind fix, thanks, but we're still having a similar problem, albeit probably in fewer circumstances.  Again, we're trying gold -r --script.  This time we perhaps need to add a little -O2 realism, fprintf instead of printf and a + 1.

martind@swiftboat:~/tmp/D116822/new$ cat main.cpp 
#include <stdio.h>

int
main() {
    fprintf(stderr, "hello world\n" + 1);
}
martind@swiftboat:~/tmp/D116822/new$ cat buildit.sh
LD=${LD:-'ld'}

g++ -O2 -g -c main.cpp 
$LD -r -o relocatable.o --script script.lnk main.o
g++ -g -o main relocatable.o 
./main
martind@swiftboat:~/tmp/D116822/new$

Works with ld, goes mad with gold:

martind@swiftboat:~/tmp/D116822/new$ bash buildit.sh 
ello world
martind@swiftboat:~/tmp/D116822/new$ LD=ld.gold bash buildit.sh 
/usr/bin/ld: relocatable.o: access beyond end of merged section (38)
;(martind@swiftboat:~/tmp/D116822/new$ 

In Ian's original fixes, he found three places in the gold source where relocatable links should use zero instead of the section address.  The results below might suggest that there's a fourth place that could use similar attention.

When built with gold, the relocations are offset by the VMA of rodata:

martind@swiftboat:~/tmp/D116822/new$ readelf --relocs relocatable.o | grep LC
000000000016  00090000000a R_X86_64_32       0000000000000026 .LC0 + 1
00000000032b  000900000001 R_X86_64_64       0000000000000026 .LC0 + 1
martind@swiftboat:~/tmp/D116822/new$ readelf --section-headers relocatable.o | grep rodata
  [ 4] .rodata           PROGBITS         0000000000000026  00000068
martind@swiftboat:~/tmp/D116822/new$ 

When built with ld, they're not:

martind@swiftboat:~/tmp/D116822/new$ readelf --relocs relocatable.o | grep LC
000000000016  00100000000a R_X86_64_32       0000000000000000 .LC0 + 1
00000000032b  001000000001 R_X86_64_64       0000000000000000 .LC0 + 1
martind@swiftboat:~/tmp/D116822/new$ readelf --section-headers relocatable.o | grep rodata
  [ 3] .rodata           PROGBITS         0000000000000026  00000066
martind@swiftboat:~/tmp/D116822/new$
Comment 1 Sourceware Commits 2015-11-25 16:51:37 UTC
The master branch has been updated by Cary Coutant <ccoutant@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=74b03b91333ccbc8fa70647eca22062e902f797f

commit 74b03b91333ccbc8fa70647eca22062e902f797f
Author: Cary Coutant <ccoutant@gmail.com>
Date:   Wed Nov 25 08:50:41 2015 -0800

    Adjust local symbol value in relocatable link to be relative to section.
    
    gold/
    	PR gold/19291
    	* object.cc (Sized_relobj_file::write_local_symbols): If relocatable,
    	subtract section address from symbol value.
Comment 2 Cary Coutant 2015-11-25 17:01:59 UTC
Fixed on trunk.

Normally, output sections in a relocatable link all start at address 0, so local symbols need no adjustment. With the linker script, however, the .rodata section is given a non-zero starting address, and the local symbol .LC0 in that section is given the wrong value.
Comment 3 Sourceware Commits 2015-12-10 13:50:54 UTC
The binutils-2_26-branch branch has been updated by Alan Modra <amodra@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=dd5474b0c1a0ae90bc68d07e18136000fde2e461

commit dd5474b0c1a0ae90bc68d07e18136000fde2e461
Author: Cary Coutant <ccoutant@gmail.com>
Date:   Wed Nov 25 08:50:41 2015 -0800

    Adjust local symbol value in relocatable link to be relative to section.
    
    gold/
    	PR gold/19291
    	* object.cc (Sized_relobj_file::write_local_symbols): If relocatable,
    	subtract section address from symbol value.
Comment 4 Sourceware Commits 2017-11-08 23:14:34 UTC
The master branch has been updated by Cary Coutant <ccoutant@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=333d0055f6f162c334c36f1946b6fcdb5c92b681

commit 333d0055f6f162c334c36f1946b6fcdb5c92b681
Author: James Clarke <jrtc27@jrtc27.com>
Date:   Wed Nov 8 15:13:53 2017 -0800

    Fix problems with -r.
    
    The fix committed for PR gold/19291 ended up breaking other cases. The
    commit added adjustment code to write_local_symbols, but in many cases
    compute_final_local_value_internal had already subtracted the output
    section's address. To fix this, all other adjustments are now removed, so
    only the one in write_local_symbols is left.
    
    gold/
    	PR gold/22266
    	* object.cc (Sized_relobj_file::compute_final_local_value_internal):
    	Drop relocatable parameter and stop adjusting output value based on
    	it.
    	(Sized_relobj_file::compute_final_local_value): Stop passing
    	relocatable to compute_final_local_value_internal.
    	(Sized_relobj_file::do_finalize_local_symbols): Ditto.
    	* object.h (Sized_relobj_file::compute_final_local_value_internal):
    	Drop relocatable parameter.
Comment 5 Sourceware Commits 2017-11-28 01:34:39 UTC
The master branch has been updated by Cary Coutant <ccoutant@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=033bfb739b525703bfe23f151d09e9beee3a2afe

commit 033bfb739b525703bfe23f151d09e9beee3a2afe
Author: Cary Coutant <ccoutant@gmail.com>
Date:   Mon Nov 27 17:32:55 2017 -0800

    Fix symbol values and relocation addends for relocatable links.
    
    The fix for PR 19291 broke some other cases where -r is used with scripts,
    as reported in PR 22266. The original fix for PR 22266 ended up breaking
    many cases for REL targets, where the addends are stored in the section data,
    and are not being adjusted properly.
    
    The problem was basically that in a relocatable output file (ET_REL),
    symbol values are supposed to be relative to the start address of their
    section. Usually in a relocatable file, all sections start at 0, so the
    failure to get this right is often irrelevant, but with a linker script,
    we occasionally see an output section whose starting address is not 0,
    and gold would occasionally write a symbol with its relocated value instead
    of its section-relative value.
    
    This patch reverts the recent fix for PR 22266 as well as my original fix
    for PR 19291. The original fix moved the symbol value adjustment to
    write_local_symbols, but neglected to undo a few places where the adjustment
    was also being applied, resulting in an occasional double adjustment. The
    more recent fix removed those other adjustments, but then failed to
    re-account for the adjustment when rewriting the relocations on REL targets.
    
    With the old attempts reverted, we now apply the symbol value adjustment to
    the one case that had been missed (non-section symbols in merge sections).
    But now we also need to account for the adjustment when rewriting the addends
    for RELA relocations.
    
    gold/
    	PR gold/19291
    	PR gold/22266
    	* object.cc (Sized_relobj_file::compute_final_local_value_internal):
    	Revert changes from 2017-11-08 patch.  Adjust symbol value in
    	relocatable links for non-section symbols.
    	(Sized_relobj_file::compute_final_local_value): Revert changes from
    	2017-11-08 patch.
    	(Sized_relobj_file::do_finalize_local_symbols): Likewise.
    	(Sized_relobj_file::write_local_symbols): Revert changes from
    	2015-11-25 patch.
    	* object.h (Sized_relobj_file::compute_final_local_value_internal):
    	Revert changes from 2017-11-08 patch.
    	* powerpc.cc (Target_powerpc::relocate_relocs): Adjust addend for
    	relocatable links.
    	* target-reloc.h (relocate_relocs): Adjust addend for relocatable links.
    	* testsuite/pr22266_a.c (hello): New function.
    	* testsuite/pr22266_main.c (main): Add test for merge sections.
    	* testsuite/pr22266_script.t: Add rule for .rodata.