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

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.

[r, 2014-07-07 22:36:24]

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

[iant, 2014-07-07 22:50:45]

LGTM

I'm not clear how this was working at all.

[lvd2, 2014-07-07 22:54:07]

i'm sure you looked at it long enough, but wouldnt the fix be to change at
the call site:

                       cfa += q->spadj;
                        putpccfadelta(q->link->pc - pc, cfa);

i must have put the increment of the cfa before the call and the increment
of the pc after the call for a reason, but the mists of time have claimed
that.

/L




2014-07-07 15:50 GMT-07:00 <iant@golang.org>:

> LGTM
>
> I'm not clear how this was working at all.
>
>
> https://codereview.appspot.com/112750043/
>

[r, 2014-07-07 22:59:01]

I don't even know what q is.

-rob


On Mon, Jul 7, 2014 at 3:54 PM, Luuk van Dijk <lvd@gmail.com> wrote:
> i'm sure you looked at it long enough, but wouldnt the fix be to change at
> the call site:
>
>                        cfa += q->spadj;
>                         putpccfadelta(q->link->pc - pc, cfa);
>
> i must have put the increment of the cfa before the call and the increment
> of the pc after the call for a reason, but the mists of time have claimed
> that.
>
> /L
>
>
>
>
> 2014-07-07 15:50 GMT-07:00 <iant@golang.org>:
>
>> LGTM
>>
>> I'm not clear how this was working at all.
>>
>>
>> https://codereview.appspot.com/112750043/
>
>

[lvd2, 2014-07-07 23:02:52]

iirc p iterates over  the chain of Prog's that make up the function pointed
to by currsym.  each one is an instruction.


2014-07-07 15:58 GMT-07:00 Rob Pike <r@golang.org>:

> I don't even know what q is.
>
> -rob
>
>
> On Mon, Jul 7, 2014 at 3:54 PM, Luuk van Dijk <lvd@gmail.com> wrote:
> > i'm sure you looked at it long enough, but wouldnt the fix be to change
> at
> > the call site:
> >
> >                        cfa += q->spadj;
> >                         putpccfadelta(q->link->pc - pc, cfa);
> >
> > i must have put the increment of the cfa before the call and the
> increment
> > of the pc after the call for a reason, but the mists of time have claimed
> > that.
> >
> > /L
> >
> >
> >
> >
> > 2014-07-07 15:50 GMT-07:00 <iant@golang.org>:
> >
> >> LGTM
> >>
> >> I'm not clear how this was working at all.
> >>
> >>
> >> https://codereview.appspot.com/112750043/
> >
> >
>

[lvd2, 2014-07-07 23:03:13]

i mean q.


2014-07-07 16:02 GMT-07:00 Luuk van Dijk <lvd@gmail.com>:

> iirc p iterates over  the chain of Prog's that make up the function
> pointed to by currsym.  each one is an instruction.
>
>
> 2014-07-07 15:58 GMT-07:00 Rob Pike <r@golang.org>:
>
> I don't even know what q is.
>>
>> -rob
>>
>>
>> On Mon, Jul 7, 2014 at 3:54 PM, Luuk van Dijk <lvd@gmail.com> wrote:
>> > i'm sure you looked at it long enough, but wouldnt the fix be to change
>> at
>> > the call site:
>> >
>> >                        cfa += q->spadj;
>> >                         putpccfadelta(q->link->pc - pc, cfa);
>> >
>> > i must have put the increment of the cfa before the call and the
>> increment
>> > of the pc after the call for a reason, but the mists of time have
>> claimed
>> > that.
>> >
>> > /L
>> >
>> >
>> >
>> >
>> > 2014-07-07 15:50 GMT-07:00 <iant@golang.org>:
>> >
>> >> LGTM
>> >>
>> >> I'm not clear how this was working at all.
>> >>
>> >>
>> >> https://codereview.appspot.com/112750043/
>> >
>> >
>>
>
>

[iant, 2014-07-07 23:05:17]

On Mon, Jul 7, 2014 at 4:03 PM, Luuk van Dijk <lvd@gmail.com> wrote:
> i mean q.

I think you may be looking at an old version of the code.  This
changed quite a bit in December with the introduction of liblink.
There is no q any more.

Ian

> 2014-07-07 16:02 GMT-07:00 Luuk van Dijk <lvd@gmail.com>:
>
>> iirc p iterates over  the chain of Prog's that make up the function
>> pointed to by currsym.  each one is an instruction.
>>
>>
>> 2014-07-07 15:58 GMT-07:00 Rob Pike <r@golang.org>:
>>
>>> I don't even know what q is.
>>>
>>> -rob
>>>
>>>
>>> On Mon, Jul 7, 2014 at 3:54 PM, Luuk van Dijk <lvd@gmail.com> wrote:
>>> > i'm sure you looked at it long enough, but wouldnt the fix be to change
>>> > at
>>> > the call site:
>>> >
>>> >                        cfa += q->spadj;
>>> >                         putpccfadelta(q->link->pc - pc, cfa);
>>> >
>>> > i must have put the increment of the cfa before the call and the
>>> > increment
>>> > of the pc after the call for a reason, but the mists of time have
>>> > claimed
>>> > that.
>>> >
>>> > /L
>>> >
>>> >
>>> >
>>> >
>>> > 2014-07-07 15:50 GMT-07:00 <iant@golang.org>:
>>> >
>>> >> LGTM
>>> >>
>>> >> I'm not clear how this was working at all.
>>> >>
>>> >>
>>> >> https://codereview.appspot.com/112750043/
>>> >
>>> >
>>
>>
>

[r, 2014-07-07 23:06:59]

I am pretty sure my fix is simple and correct.

-rob


On Mon, Jul 7, 2014 at 4:05 PM, Ian Lance Taylor <iant@golang.org> wrote:
> On Mon, Jul 7, 2014 at 4:03 PM, Luuk van Dijk <lvd@gmail.com> wrote:
>> i mean q.
>
> I think you may be looking at an old version of the code.  This
> changed quite a bit in December with the introduction of liblink.
> There is no q any more.
>
> Ian
>
>> 2014-07-07 16:02 GMT-07:00 Luuk van Dijk <lvd@gmail.com>:
>>
>>> iirc p iterates over  the chain of Prog's that make up the function
>>> pointed to by currsym.  each one is an instruction.
>>>
>>>
>>> 2014-07-07 15:58 GMT-07:00 Rob Pike <r@golang.org>:
>>>
>>>> I don't even know what q is.
>>>>
>>>> -rob
>>>>
>>>>
>>>> On Mon, Jul 7, 2014 at 3:54 PM, Luuk van Dijk <lvd@gmail.com> wrote:
>>>> > i'm sure you looked at it long enough, but wouldnt the fix be to change
>>>> > at
>>>> > the call site:
>>>> >
>>>> >                        cfa += q->spadj;
>>>> >                         putpccfadelta(q->link->pc - pc, cfa);
>>>> >
>>>> > i must have put the increment of the cfa before the call and the
>>>> > increment
>>>> > of the pc after the call for a reason, but the mists of time have
>>>> > claimed
>>>> > that.
>>>> >
>>>> > /L
>>>> >
>>>> >
>>>> >
>>>> >
>>>> > 2014-07-07 15:50 GMT-07:00 <iant@golang.org>:
>>>> >
>>>> >> LGTM
>>>> >>
>>>> >> I'm not clear how this was working at all.
>>>> >>
>>>> >>
>>>> >> https://codereview.appspot.com/112750043/
>>>> >
>>>> >
>>>
>>>
>>

[dfc, 2014-07-07 23:07:26]

Should this be backported to 1.3.1 ?

On Tue, Jul 8, 2014 at 8:50 AM,  <iant@golang.org> wrote:
> LGTM
>
> I'm not clear how this was working at all.
>
>
> https://codereview.appspot.com/112750043/
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[r, 2014-07-07 23:07:29]

*** 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 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.

LGTM=iant
R=rsc, iant, lvd
CC=golang-codereviews
https://codereview.appspot.com/112750043

[r, 2014-07-07 23:08:43]

Re: Backporting: No. Nobody depends on this data except maybe gdb, and
it doesn't work anyway.

-rob


On Mon, Jul 7, 2014 at 4:07 PM,  <r@golang.org> wrote:
> *** 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 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.
>
> LGTM=iant
> R=rsc, iant, lvd
> CC=golang-codereviews
> https://codereview.appspot.com/112750043
>
>
> https://codereview.appspot.com/112750043/

[lvd2, 2014-07-07 23:09:10]

LGTM
looking at a non-stale version of dwarf.c, i am pretty sure too.


2014-07-07 16:06 GMT-07:00 Rob Pike <r@golang.org>:

> I am pretty sure my fix is simple and correct.
>
> -rob
>
>
> On Mon, Jul 7, 2014 at 4:05 PM, Ian Lance Taylor <iant@golang.org> wrote:
> > On Mon, Jul 7, 2014 at 4:03 PM, Luuk van Dijk <lvd@gmail.com> wrote:
> >> i mean q.
> >
> > I think you may be looking at an old version of the code.  This
> > changed quite a bit in December with the introduction of liblink.
> > There is no q any more.
> >
> > Ian
> >
> >> 2014-07-07 16:02 GMT-07:00 Luuk van Dijk <lvd@gmail.com>:
> >>
> >>> iirc p iterates over  the chain of Prog's that make up the function
> >>> pointed to by currsym.  each one is an instruction.
> >>>
> >>>
> >>> 2014-07-07 15:58 GMT-07:00 Rob Pike <r@golang.org>:
> >>>
> >>>> I don't even know what q is.
> >>>>
> >>>> -rob
> >>>>
> >>>>
> >>>> On Mon, Jul 7, 2014 at 3:54 PM, Luuk van Dijk <lvd@gmail.com> wrote:
> >>>> > i'm sure you looked at it long enough, but wouldnt the fix be to
> change
> >>>> > at
> >>>> > the call site:
> >>>> >
> >>>> >                        cfa += q->spadj;
> >>>> >                         putpccfadelta(q->link->pc - pc, cfa);
> >>>> >
> >>>> > i must have put the increment of the cfa before the call and the
> >>>> > increment
> >>>> > of the pc after the call for a reason, but the mists of time have
> >>>> > claimed
> >>>> > that.
> >>>> >
> >>>> > /L
> >>>> >
> >>>> >
> >>>> >
> >>>> >
> >>>> > 2014-07-07 15:50 GMT-07:00 <iant@golang.org>:
> >>>> >
> >>>> >> LGTM
> >>>> >>
> >>>> >> I'm not clear how this was working at all.
> >>>> >>
> >>>> >>
> >>>> >> https://codereview.appspot.com/112750043/
> >>>> >
> >>>> >
> >>>
> >>>
> >>
>

[gobot, 2014-07-07 23:26:42]

This CL appears to have broken the windows-386 builder.
See http://build.golang.org/log/79ab2bf1308e595ba359479fde2aca9171af1bc5

[dfc, 2014-07-07 23:39:43]

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