Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1843)

Issue 4808043: code review 4808043: ld: remove overlap of ELF sections on dynamic binaries (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years ago by niemeyer
Modified:
8 years ago
Reviewers:
gustavo, r, r2
CC:
rsc, golang-dev
Visibility:
Public.

Description

ld: remove overlap of ELF sections on dynamic binaries The dynamic ELF sections were pointing to the proper data, but that data was already owned by the rodata and text sections. Some ELF references explicitly prohibit multiple sections from owning the same data, and strip behaves accordingly. The data for these sections was moved out and their ranges are now owned by their respective sections. This change makes strip happy both with and without -s being provided at link time. A test was added in debug/elf to ensure there are no regressions on this area in the future. Fixes issue 1242. Fixes issue 2022. NOTE: Tested on Linux amd64/386/arm only.

Patch Set 1 #

Patch Set 2 : code review 4808043: ld: remove overlap of ELF sections on dynamic binaries #

Patch Set 3 : diff -r 89c7137ea35c https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 89c7137ea35c https://go.googlecode.com/hg/ #

Total comments: 9

Patch Set 5 : diff -r 89c7137ea35c https://go.googlecode.com/hg/ #

Patch Set 6 : diff -r fd042c825740 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+185 lines, -123 lines) Patch
M src/cmd/5l/asm.c View 1 2 3 4 10 chunks +28 lines, -24 lines 0 comments Download
M src/cmd/5l/span.c View 1 2 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/6l/asm.c View 1 2 3 4 10 chunks +42 lines, -23 lines 0 comments Download
M src/cmd/8l/asm.c View 1 2 3 4 10 chunks +38 lines, -34 lines 0 comments Download
M src/cmd/ld/data.c View 1 2 3 chunks +37 lines, -11 lines 0 comments Download
M src/cmd/ld/elf.c View 1 2 4 chunks +7 lines, -28 lines 0 comments Download
M src/cmd/ld/lib.h View 1 2 2 chunks +2 lines, -1 line 0 comments Download
M src/cmd/ld/pe.c View 1 2 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/debug/elf/file_test.go View 1 2 2 chunks +29 lines, -0 lines 0 comments Download

Messages

Total messages: 13
niemeyer
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
8 years ago (2011-07-20 13:22:04 UTC) #1
rsc
http://codereview.appspot.com/4808043/diff/6001/src/cmd/5l/asm.c File src/cmd/5l/asm.c (left): http://codereview.appspot.com/4808043/diff/6001/src/cmd/5l/asm.c#oldcode638 src/cmd/5l/asm.c:638: diag("ELFRESERVE too small: %d > %d", a, ELFRESERVE); Why ...
8 years ago (2011-07-20 14:51:36 UTC) #2
niemeyer
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
8 years ago (2011-07-20 15:42:25 UTC) #3
niemeyer
All addressed, and also fixed a bug in 5l which had .got as read-only.
8 years ago (2011-07-20 15:43:12 UTC) #4
rsc
LGTM
8 years ago (2011-07-20 15:43:54 UTC) #5
niemeyer
*** Submitted as http://code.google.com/p/go/source/detail?r=5fbd69feecd3 *** ld: remove overlap of ELF sections on dynamic binaries The ...
8 years ago (2011-07-20 15:47:08 UTC) #6
gustavo_niemeyer.net
> gopack grc _test/crypto/tls.a _gotest_.6 > gotest: ./6.out failed to start: fork/exec ./6.out: exec format ...
8 years ago (2011-07-20 16:54:35 UTC) #7
r
It's buggering the headers if the program links in cgo code. It looks like the ...
8 years ago (2011-07-21 01:29:26 UTC) #8
gustavo_niemeyer.net
> It's buggering the headers if the program links in cgo code. It looks > ...
8 years ago (2011-07-21 01:56:11 UTC) #9
r2
It could be both, of course. I believe FreeBSD needs more stuff to be in ...
8 years ago (2011-07-21 01:58:56 UTC) #10
gustavo_niemeyer.net
> It could be both, of course. I believe FreeBSD needs more stuff to be ...
8 years ago (2011-07-21 03:33:17 UTC) #11
gustavo_niemeyer.net
> I'll leave the .shstrtab alone for now, as it seems to not care > ...
8 years ago (2011-07-21 06:04:42 UTC) #12
r2
8 years ago (2011-07-21 06:14:43 UTC) #13
seems to fix it for amd64 freebsd 8.0

-rob

Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b