code review 100870044: runtime: make continuation pc available to stack walk

runtime: make continuation pc available to stack walk

The 'continuation pc' is where the frame will continue
execution, if anywhere. For a frame that stopped execution
due to a CALL instruction, the continuation pc is immediately
after the CALL. But for a frame that stopped execution due to
a fault, the continuation pc is the pc after the most recent CALL
to deferproc in that frame, or else 0. That is where execution
will continue, if anywhere.

The liveness information is only recorded for CALL instructions.
This change makes sure that we never look for liveness information
except for CALL instructions.

Using a valid PC fixes crashes when a garbage collection or
stack copying tries to process a stack frame that has faulted.

Record continuation pc in heapdump (format change).

Fixes issue 8048.

[rsc, 2014-05-30 20:42:26]

Hello khr (cc: golang-codereviews@googlegroups.com, iant, r),

I'd like you to review this change to
https://code.google.com/p/go/

[rsc, 2014-05-30 20:43:58]

This is x86-only right now, but I wanted to get the review going. I will
work on the ARM version in the next day or so (my Chromebook is at home).

[iant, 2014-05-30 21:50:09]

LGTM though I'm not sure I see all the ramifications.

https://codereview.appspot.com/100870044/diff/100001/src/pkg/runtime/heapdump.c
File src/pkg/runtime/heapdump.c (right):

https://codereview.appspot.com/100870044/diff/100001/src/pkg/runtime/heapdump...
src/pkg/runtime/heapdump.c:349: dumpint(s->continpc);
Somebody needs to update https://code.google.com/p/go-wiki/wiki/heapdump13

https://codereview.appspot.com/100870044/diff/100001/src/pkg/runtime/panic.c
File src/pkg/runtime/panic.c (right):

https://codereview.appspot.com/100870044/diff/100001/src/pkg/runtime/panic.c#...
src/pkg/runtime/panic.c:223: p.defer = nil;
Why bother?  We just did a memclr of the entire struct.

[rsc, 2014-05-30 22:36:40]

https://codereview.appspot.com/100870044/diff/100001/src/pkg/runtime/heapdump.c
File src/pkg/runtime/heapdump.c (right):

https://codereview.appspot.com/100870044/diff/100001/src/pkg/runtime/heapdump...
src/pkg/runtime/heapdump.c:349: dumpint(s->continpc);
On 2014/05/30 21:50:09, iant wrote:
> Somebody needs to update https://code.google.com/p/go-wiki/wiki/heapdump13

Yes, I'll do that once Keith signs off on this.

https://codereview.appspot.com/100870044/diff/100001/src/pkg/runtime/panic.c
File src/pkg/runtime/panic.c (right):

https://codereview.appspot.com/100870044/diff/100001/src/pkg/runtime/panic.c#...
src/pkg/runtime/panic.c:223: p.defer = nil;
On 2014/05/30 21:50:09, iant wrote:
> Why bother?  We just did a memclr of the entire struct.

So we did; I missed that. There was a mysterious crash and I thought this might
be it (it didn't help, obviously). Will revert.

[rsc, 2014-05-31 01:17:28]

PTAL

linux/arm, linux/amd64, linux/386, darwin/amd64, darwin/386 all tested and
pass

[khr, 2014-05-31 04:36:33]

LGTM.

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/traceback...
File src/pkg/runtime/traceback_arm.c (right):

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/traceback...
src/pkg/runtime/traceback_arm.c:21: Stkframe frame, last;
last is unused, remove it.

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/traceback...
src/pkg/runtime/traceback_arm.c:164: sparg += 3*sizeof(uintreg);
Shouldn't this be += 2*sizeof(uintreg)?

[dvyukov, 2014-05-31 12:05:26]

I think this approach is more promising than altering codegen and live maps, we
indeed don't need any additional info for faulting frames as we will skip them
anyway.

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/stack.c
File src/pkg/runtime/stack.c (right):

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:495: runtime·printf("    adjusting %s frame=[%p,%p]
pc=%p\n", runtime·funcname(f), frame->sp, frame->fp, frame->pc);
I think it's better to print continpc here, as this function works with continpc
(or print both)

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/traceback...
File src/pkg/runtime/traceback_x86.c (right):

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/traceback...
src/pkg/runtime/traceback_x86.c:67: while(defer != nil && defer->argp ==
(byte*)-1)
please replace -1 with a new named constant, here, below and in proc.c and
cgocall.c
previously it was merely "crash if try to use this sp", but now it is also a
special value checked in other places

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/traceback...
src/pkg/runtime/traceback_x86.c:231: if(panic != nil && panic->defer->argp ==
(byte*)sparg)
what if the goroutine is preempted not right at sigpanic, but a bit later, when
runtime.panic runs defers?
a defer can be already unlinked from gp->defer and be executing
this code will miss such defer and set continpc to 0
but later the defer can recover and continue execution at the frame for which we
recorded continpc=0

is it possible? does this code handle this? can it affect GC/stackcopy?

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/traceback...
src/pkg/runtime/traceback_x86.c:307: if(callback == nil && n < max && (defer !=
nil || panic != nil)) {
why callback == nil?
we usually throw in this function if callback != nil, i.e. when tracing back for
GC or stack copy, because it's critical to have correct information in these
contexts
also, we usually print inconsistencies always, so that bugs are noticed earler

[rsc, 2014-05-31 13:58:31]

Retesting and will submit. Will also update wiki page.

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/stack.c
File src/pkg/runtime/stack.c (right):

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:495: runtime·printf("    adjusting %s frame=[%p,%p]
pc=%p\n", runtime·funcname(f), frame->sp, frame->fp, frame->pc);
On 2014/05/31 12:05:26, dvyukov wrote:
> I think it's better to print continpc here, as this function works with
continpc
> (or print both)

Done.

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/traceback...
File src/pkg/runtime/traceback_arm.c (right):

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/traceback...
src/pkg/runtime/traceback_arm.c:21: Stkframe frame, last;
On 2014/05/31 04:36:32, khr wrote:
> last is unused, remove it.

Done.

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/traceback...
src/pkg/runtime/traceback_arm.c:164: sparg += 3*sizeof(uintreg);
On 2014/05/31 04:36:32, khr wrote:
> Shouldn't this be += 2*sizeof(uintreg)?

No, on ARM there are three words. On x86 the stack frame looks like

[args for outgoing call] [local vars] [return address] [func args]

but on ARM (the way we use it) a stack frame looks like:

[return address] [args for outgoing call] [local vars] [caller's return address]
[func args]

That is, we save the LR in the prologue at the bottom of the stack instead of at
the top. There's an open bug to change this to match x86 and gcc arm, and I got
most of the way through it once but never finished and committed it.

For newproc/deferproc, x86 sets up the call as normal (as if to call it instead
of to go/defer it) and then pushes the func value and the arg size:

[arg size] [func] [args for outgoing call] [local vars] [return address] [func
args]

and then it calls newproc/deferproc as if [arg size] [func] [args for outgoing
call] were all 'args'.
But ARM has to make the frame look like a standard frame, which means pushing
another word onto the stack:

[return address] [arg size] [func] [return address] [args for outgoing call]
[local vars] [caller's return address] [func args]

newproc/deferproc know to skip over the return address between func and 'args
for outgoing call' on ARM.

I'd like to clean this up and make all the architectures look the same as far as
the stack is concerned. Doing so would let us unify the traceback routines too.
But for now it's 3 on ARM.

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/traceback...
File src/pkg/runtime/traceback_x86.c (right):

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/traceback...
src/pkg/runtime/traceback_x86.c:67: while(defer != nil && defer->argp ==
(byte*)-1)
On 2014/05/31 12:05:26, dvyukov wrote:
> please replace -1 with a new named constant, here, below and in proc.c and
> cgocall.c
> previously it was merely "crash if try to use this sp", but now it is also a
> special value checked in other places

Done.

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/traceback...
src/pkg/runtime/traceback_x86.c:231: if(panic != nil && panic->defer->argp ==
(byte*)sparg)
On 2014/05/31 12:05:26, dvyukov wrote:
> what if the goroutine is preempted not right at sigpanic, but a bit later,
when
> runtime.panic runs defers?
> a defer can be already unlinked from gp->defer and be executing
> this code will miss such defer and set continpc to 0
> but later the defer can recover and continue execution at the frame for which
we
> recorded continpc=0
> 
> is it possible? does this code handle this? can it affect GC/stackcopy?

Yes, that is possible. Yes, the code handles it. In fact it handles it on this
very line!

The problem you are describing is a problem with line 233 (gp->defer may miss a
defer).
Looking at gp->panic too (on this line) means that we do not miss it after all.

https://codereview.appspot.com/100870044/diff/80002/src/pkg/runtime/traceback...
src/pkg/runtime/traceback_x86.c:307: if(callback == nil && n < max && (defer !=
nil || panic != nil)) {
On 2014/05/31 12:05:26, dvyukov wrote:
> why callback == nil?
> we usually throw in this function if callback != nil, i.e. when tracing back
for
> GC or stack copy, because it's critical to have correct information in these
> contexts
> also, we usually print inconsistencies always, so that bugs are noticed earler

Thanks, this is a copy-paste error. The arm version got it right: callback !=
nil. We only care about getting it perfect when we're reporting back for garbage
collection or stack copying. In other contexts we may not be able to get it
perfect at all. For example, profiling calls gentraceback at times when the
stack may not be possible to walk all the way back. We don't care because we
just lose a profile report, but it's a real reason the walk can stop early.
Apparently it didn't happen on the linux or darwin x86 machines I tested with,
but it does happen on linux/arm and I suspect it happens on Windows too, because
of the different way we do Windows profiling.

[rsc, 2014-05-31 14:09:46]

*** Submitted as https://code.google.com/p/go/source/detail?r=ae4861ef1935 ***

runtime: make continuation pc available to stack walk

The 'continuation pc' is where the frame will continue
execution, if anywhere. For a frame that stopped execution
due to a CALL instruction, the continuation pc is immediately
after the CALL. But for a frame that stopped execution due to
a fault, the continuation pc is the pc after the most recent CALL
to deferproc in that frame, or else 0. That is where execution
will continue, if anywhere.

The liveness information is only recorded for CALL instructions.
This change makes sure that we never look for liveness information
except for CALL instructions.

Using a valid PC fixes crashes when a garbage collection or
stack copying tries to process a stack frame that has faulted.

Record continuation pc in heapdump (format change).

Fixes issue 8048.

LGTM=iant, khr
R=khr, iant, dvyukov
CC=golang-codereviews, r
https://codereview.appspot.com/100870044

[gobot, 2014-05-31 14:27:29]

This CL appears to have broken the windows-amd64-race builder.
See http://build.golang.org/log/ef44b289a4baabbd51acbfb4fdd2a64bc837ac58

[bradfitz, 2014-05-31 21:54:32]

Real?
 On May 31, 2014 11:27 PM, <gobot@golang.org> wrote:

> This CL appears to have broken the windows-amd64-race builder.
> See http://build.golang.org/log/ef44b289a4baabbd51acbfb4fdd2a64bc837ac58
>
> https://codereview.appspot.com/100870044/
>
> --
> 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.
>

[rsc, 2014-05-31 23:00:29]

Yes, looks like a real breakage. Will look into it.


On Sat, May 31, 2014 at 5:54 PM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:

> Real?
>  On May 31, 2014 11:27 PM, <gobot@golang.org> wrote:
>
>> This CL appears to have broken the windows-amd64-race builder.
>> See http://build.golang.org/log/ef44b289a4baabbd51acbfb4fdd2a64bc837ac58
>>
>> https://codereview.appspot.com/100870044/
>>
>> --
>> 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.
>>
>