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

Issue 112750043: code review 112750043: cmd/ld: fix off-by-one in DWARF frame tables (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 4 months ago by r
Modified:
7 years, 4 months ago
Reviewers:
gobot, dfc, iant, lvd2
CC:
rsc, iant, lvd1, lvd2, golang-codereviews
Visibility:
Public.

Description

cmd/ld: fix off-by-one in DWARF frame tables The code generating the .debug_frame section emits pairs of "advance PC", "set SP offset" pseudo-instructions. Before the fix, the PC advance comes out before the SP setting, which means the emitted offset for a block is actually the value at the end of the block, which is incorrect for the block itself. The easiest way to fix this problem is to emit the SP offset before the PC advance. One delicate point: the last instruction to come out is now an "advance PC", which means that if there are padding intsructions after the final RET, they will appear to have a non-zero offset. This is odd but harmless because there is no legal way to have a PC in that range, or to put it another way, if you get here the SP is certainly screwed up so getting the wrong (virtual) frame pointer is the least of your worries.

Patch Set 1 #

Patch Set 2 : diff -r 690153652172 https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -3 lines) Patch
M src/cmd/ld/dwarf.c View 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 14
r
Hello rsc, iant, lvd@golang.org (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
7 years, 4 months ago (2014-07-07 22:36:24 UTC) #1
iant
LGTM I'm not clear how this was working at all.
7 years, 4 months ago (2014-07-07 22:50:45 UTC) #2
lvd2
i'm sure you looked at it long enough, but wouldnt the fix be to change ...
7 years, 4 months ago (2014-07-07 22:54:07 UTC) #3
r
I don't even know what q is. -rob On Mon, Jul 7, 2014 at 3:54 ...
7 years, 4 months ago (2014-07-07 22:59:01 UTC) #4
lvd2
iirc p iterates over the chain of Prog's that make up the function pointed to ...
7 years, 4 months ago (2014-07-07 23:02:52 UTC) #5
lvd2
i mean q. 2014-07-07 16:02 GMT-07:00 Luuk van Dijk <lvd@gmail.com>: > iirc p iterates over ...
7 years, 4 months ago (2014-07-07 23:03:13 UTC) #6
iant
On Mon, Jul 7, 2014 at 4:03 PM, Luuk van Dijk <lvd@gmail.com> wrote: > i ...
7 years, 4 months ago (2014-07-07 23:05:17 UTC) #7
r
I am pretty sure my fix is simple and correct. -rob On Mon, Jul 7, ...
7 years, 4 months ago (2014-07-07 23:06:59 UTC) #8
dfc
Should this be backported to 1.3.1 ? On Tue, Jul 8, 2014 at 8:50 AM, ...
7 years, 4 months ago (2014-07-07 23:07:26 UTC) #9
r
*** Submitted as https://code.google.com/p/go/source/detail?r=d23b0ca920ca *** cmd/ld: fix off-by-one in DWARF frame tables The code generating ...
7 years, 4 months ago (2014-07-07 23:07:29 UTC) #10
r
Re: Backporting: No. Nobody depends on this data except maybe gdb, and it doesn't work ...
7 years, 4 months ago (2014-07-07 23:08:43 UTC) #11
lvd2
LGTM looking at a non-stale version of dwarf.c, i am pretty sure too. 2014-07-07 16:06 ...
7 years, 4 months ago (2014-07-07 23:09:10 UTC) #12
gobot
This CL appears to have broken the windows-386 builder. See http://build.golang.org/log/79ab2bf1308e595ba359479fde2aca9171af1bc5
7 years, 4 months ago (2014-07-07 23:26:42 UTC) #13
dfc
7 years, 4 months ago (2014-07-07 23:39:43 UTC) #14
On Tue, Jul 8, 2014 at 9:26 AM,  <gobot@golang.org> wrote:
> This CL appears to have broken the windows-386 builder.
> See http://build.golang.org/log/79ab2bf1308e595ba359479fde2aca9171af1bc5

Likely unrelated

# ..\misc\cgo\test
--- FAIL: TestParallelSleep (1.77s)
issue1560.go:44: difference in start time for two sleep(1) is 531ms
issue1560.go:48: parallel 1-second sleeps slept for 0.531000 seconds
FAIL
scatter = 004F6A5C
hello from C
sqrt is: 0
FAIL _/c_/gobuilder/windows-386-d23b0ca920ca/go/misc/cgo/test 4.265s
Sign in to reply to this message.

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