code review 10169045: runtime: adjust traceback / garbage collector boundary

runtime: adjust traceback / garbage collector boundary

The garbage collection routine addframeroots is duplicating
logic in the traceback routine that calls it, sometimes correctly,
sometimes incorrectly, sometimes incompletely.
Pass necessary information to addframeroots instead of
deriving it anew.

Should make addframeroots significantly more robust.
It's certainly smaller.

Also try to standardize on uintptr for saved pc, sp values.

Will make CL 10036044 trivial.

[rsc, 2013-06-12 02:36:31]

Hello golang-dev@googlegroups.com,

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

[dave_cheney.net, 2013-06-12 02:45:15]

https://codereview.appspot.com/10169045/diff/7001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/10169045/diff/7001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1456: if(0 && ScanStackByFrames && pc ==
(uintptr)runtime·goexit && gp->fnstart != nil) {
is this left over debugging ?

[rsc, 2013-06-12 02:57:52]

https://codereview.appspot.com/10169045/diff/7001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/10169045/diff/7001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1456: if(0 && ScanStackByFrames && pc ==
(uintptr)runtime·goexit && gp->fnstart != nil) {
On 2013/06/12 02:45:15, dfc wrote:
> is this left over debugging ?

No, the whole block is dead code that I meant to remove. Done. Thanks.

[dvyukov, 2013-06-12 11:30:14]

LGTM
I like the GC simplification

https://codereview.appspot.com/10169045/diff/13001/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/10169045/diff/13001/src/pkg/runtime/runtime.h#...
src/pkg/runtime/runtime.h:660: Func *fn;	// function being run
stick * to type

https://codereview.appspot.com/10169045/diff/13001/src/pkg/runtime/runtime.h#...
src/pkg/runtime/runtime.h:660: Func *fn;	// function being run
tab between type and name?

https://codereview.appspot.com/10169045/diff/13001/src/pkg/runtime/runtime.h#...
src/pkg/runtime/runtime.h:667: byte *varp;	// pointer to local variables
stick * to type

https://codereview.appspot.com/10169045/diff/13001/src/pkg/runtime/runtime.h#...
src/pkg/runtime/runtime.h:741: void	runtime·traceback(uintptr pc, uintptr sp,
uintptr lr, G* gp);
shouldn't this be moved into "stack traces" section as well?

https://codereview.appspot.com/10169045/diff/13001/src/pkg/runtime/traceback_...
File src/pkg/runtime/traceback_x86.c (right):

https://codereview.appspot.com/10169045/diff/13001/src/pkg/runtime/traceback_...
src/pkg/runtime/traceback_x86.c:22: // collector (callback != nil).  A little
clunky to merge the two but avoids
s/two/three/

[rsc, 2013-06-12 12:44:53]

Done, thanks.

[rsc, 2013-06-12 12:49:41]

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

runtime: adjust traceback / garbage collector boundary

The garbage collection routine addframeroots is duplicating
logic in the traceback routine that calls it, sometimes correctly,
sometimes incorrectly, sometimes incompletely.
Pass necessary information to addframeroots instead of
deriving it anew.

Should make addframeroots significantly more robust.
It's certainly smaller.

Also try to standardize on uintptr for saved pc, sp values.

Will make CL 10036044 trivial.

R=golang-dev, dave, dvyukov
CC=golang-dev
https://codereview.appspot.com/10169045

[iant, 2013-06-12 13:24:51]

LGTM

https://codereview.appspot.com/10169045/diff/17002/src/pkg/runtime/traceback_...
File src/pkg/runtime/traceback_arm.c (right):

https://codereview.appspot.com/10169045/diff/17002/src/pkg/runtime/traceback_...
src/pkg/runtime/traceback_arm.c:97: frame.arglen = 2*sizeof(uintptr) +
((uintptr*)frame.argp)[1];
Both deferproc and newproc take an int32 as their first argument.  Is (uintptr*)
correct here?  Why not
    *(int32*)(frame.argp+PtrSize)
?  I suppose since this is traceback_arm it doesn't make a difference.

https://codereview.appspot.com/10169045/diff/17002/src/pkg/runtime/traceback_...
File src/pkg/runtime/traceback_x86.c (right):

https://codereview.appspot.com/10169045/diff/17002/src/pkg/runtime/traceback_...
src/pkg/runtime/traceback_x86.c:106: frame.arglen = 2*sizeof(uintptr) +
((uintptr*)frame.argp)[1];
Again, argument is int32.

[rsc, 2013-06-12 13:48:20]

Thanks for pointing out the newproc inconsistency. The arguments are pushed
as machine words, so int32 is probably the wrong type in the prototype. But
it's easy enough to be consistent there and say that it's the compiler that
is making assumptions (namely that pushing a small int64 will read
correctly as an int32). However, having to look that up reminded me that
the count is first, not second, so the [1] is wrong.

Russ

[ality, 2013-06-12 14:33:28]

This isn't really related to the CL, but I'm curious about the two
two significant differences in the traceback code for arm and x86:

1. Why does the x86 code additionally check that frame.{pc sp} are
   equal to gp->sched.{pc sp} when testing for pc == runtime·goexit?

2. Why doesn't the arm code have a test for pc == runtime·morestack?

Thanks,
  Anthony

[rsc, 2013-06-12 15:53:44]

On Wed, Jun 12, 2013 at 10:33 AM, Anthony Martin <ality@pbrane.org> wrote:

> This isn't really related to the CL, but I'm curious about the two
> two significant differences in the traceback code for arm and x86:
>
> 1. Why does the x86 code additionally check that frame.{pc sp} are
>    equal to gp->sched.{pc sp} when testing for pc == runtime·goexit?
>

I've already deleted that code in my local copy, so I'm not going to bother
figuring out the answer.


> 2. Why doesn't the arm code have a test for pc == runtime·morestack?
>

It's essentially a complicated debugging print. It says "if we're unwinding
an m stack and we see morestack, then jump to the corresponding g and start
unwinding that too, so you can see what function in the g ran out of stack
space". That logic is only for printing; in particular it is not used for
obtaining traces for profiling or garbage collection. The logic could be
added to arm too; it just never was.

Russ

[albert.strasheim, 2013-06-14 11:30:25]

Hello

We're seeing a crash that dvyukov suggested might be related to this CL.

runtime: unknown argument frame size for runtime.copy
fatal error: invalid stack

goroutine 0 [idle]:
runtime: unknown argument frame size for runtime.sigreturn
panic during panic

No stack trace beyond that.

I suspect it's happening in code with quite a deep stack that is using the copy
builtin on some memory mapped with syscall.Mmap.

We've seen crashes with:

linux/amd64 devel +e274daf75c26 Thu Jun 13 17:04:47 2013 -0700
linux/amd64 devel +52d53d0e177e Fri Jun 14 11:14:45 2013 +0200

We also saw one segfault in the same test a few days ago while running:

linux/amd64 devel +ae79f385177d Wed Jun 12 14:05:13 2013 +1000

It didn't produce a stack trace in that case either.

It's quite a large chunk of code, but it hasn't seen any changes in a while, so
I think it's probably the runtime, not us.

If it would help we can try and reduce it to something we can submit to the
issue tracker. It's going to take us a while, so if anyone has any ideas before
we start, that would be nice.

Cheers

Albert