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

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:
6 years, 4 months ago by niemeyer
Modified:
6 years, 4 months 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/
6 years, 4 months 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 ...
6 years, 4 months ago (2011-07-20 14:51:36 UTC) #2
niemeyer
Hello rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
6 years, 4 months 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.
6 years, 4 months ago (2011-07-20 15:43:12 UTC) #4
rsc
LGTM
6 years, 4 months 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 ...
6 years, 4 months 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 ...
6 years, 4 months 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 ...
6 years, 4 months 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 > ...
6 years, 4 months 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 ...
6 years, 4 months 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 ...
6 years, 4 months 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 > ...
6 years, 4 months ago (2011-07-21 06:04:42 UTC) #12
r2
6 years, 4 months 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 c59f3c4