code review 8022044: cmd/ld, runtime: restrict stack root scan to locals and...

cmd/ld, runtime: restrict stack root scan to locals and arguments

Updates issue 5134

[cshapiro, 2013-03-26 22:52:39]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-03-26 23:13:17]

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

https://codereview.appspot.com/8022044/diff/1002/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:1320: // scanned activation has an unknown argument size.
 When doframe is true the
"doframe is true" is a confusing comment for a type declared as "void *".

Why isn't it bool*? Assembler confused? Then the comment could say "When
*doframe is true".

[bradfitz, 2013-03-26 23:14:42]

How much of the stack-GC story is this?  Could you reference a bug number
telling more of the story/plan? For instance, when can we kill the -Z link
option?

On Tue, Mar 26, 2013 at 3:52 PM, <cshapiro@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> cmd/ld, runtime: restrict stack root scan to locals and arguments
>
> Please review this at
https://codereview.appspot.**com/8022044/<https://codereview.appspot.com/8022...
>
> Affected files:
>   M src/cmd/ld/lib.c
>   M src/pkg/runtime/mgc0.c
>   M src/pkg/runtime/mprof.goc
>   M src/pkg/runtime/proc.c
>   M src/pkg/runtime/runtime.h
>   M src/pkg/runtime/symtab.c
>   M src/pkg/runtime/traceback_arm.**c
>   M src/pkg/runtime/traceback_x86.**c
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[cshapiro1, 2013-03-26 23:24:57]

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

https://codereview.appspot.com/8022044/diff/1002/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:1320: // scanned activation has an unknown argument size.
 When doframe is true the
The last argument is a generic context argument for the new gentraceback
callback.  In this particular function it is interpreted as a pointer to bool.

I will update the comment to say *doframe to avoid some confusion.

Conceivably, I could type doframe as a bool* but that would require a long and
unsightly cast at its use in the call to gentraceback on line 1384.

[cshapiro1, 2013-03-26 23:32:54]

On 2013/03/26 23:14:42, bradfitz wrote:
> How much of the stack-GC story is this?  Could you reference a bug number
> telling more of the story/plan? For instance, when can we kill the -Z link
> option?

This is the first step toward scanning live pointers and ignoring dead and
non-pointer values on the stack.  This change allows the garbage collector to
operate on a per-frame basis and uses the only helpful metadata regarding frames
available now which is the size of the various regions of a frame.  We cannot
eliminate -Z yet but I hope that will mostly change in two or three changelists
down the line when more relevant metadata is emitted.

I can add an updates issue 909 to the change since that issue is relevant.  If
filing a specific bug against the problem this change is working toward
addressing (non-pointer and dead pointer values are scanned) I can create that
and an "update issue ..." for that.

Let me know.

[cshapiro1, 2013-03-27 00:20:30]

(BTW, PTAL)

[bradfitz, 2013-03-27 00:30:01]

909 is very high-profile.  I'd suggest your own stack-specific bug.

On Tue, Mar 26, 2013 at 4:32 PM, <cshapiro@google.com> wrote:

> On 2013/03/26 23:14:42, bradfitz wrote:
>
>> How much of the stack-GC story is this?  Could you reference a bug
>>
> number
>
>> telling more of the story/plan? For instance, when can we kill the -Z
>>
> link
>
>> option?
>>
>
> This is the first step toward scanning live pointers and ignoring dead
> and non-pointer values on the stack.  This change allows the garbage
> collector to operate on a per-frame basis and uses the only helpful
> metadata regarding frames available now which is the size of the various
> regions of a frame.  We cannot eliminate -Z yet but I hope that will
> mostly change in two or three changelists down the line when more
> relevant metadata is emitted.
>
> I can add an updates issue 909 to the change since that issue is
> relevant.  If filing a specific bug against the problem this change is
> working toward addressing (non-pointer and dead pointer values are
> scanned) I can create that and an "update issue ..." for that.
>
> Let me know.
>
>
>
https://codereview.appspot.**com/8022044/<https://codereview.appspot.com/8022...
>

[cshapiro1, 2013-03-27 00:41:25]

I have created an issue and made note of it in the change description.

[bradfitz, 2013-03-27 16:27:07]

Thanks.

To be clear (and you know this, but for others): I'm not qualified to
review this.  I was just skimming for curiosity.

With Russ currently away, I'm not sure who would be a good reviewer.

I'm also unsure whether this can even go in before Go 1.1.  This feels
scary to me, especially considering we already have a few mystery crashes
in the runtime (GC/scheduler/netpoll/maps?)  How confident in it are you?


On Tue, Mar 26, 2013 at 5:41 PM, <cshapiro@google.com> wrote:

> I have created an issue and made note of it in the change description.
>
>
https://codereview.appspot.**com/8022044/<https://codereview.appspot.com/8022...
>

[DMorsing, 2013-03-27 19:43:51]

I can take a shot at reviewing this. Dmitri can probably also take a look.

https://codereview.appspot.com/8022044/diff/14001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/8022044/diff/14001/src/cmd/ld/lib.c#newcode1864
src/cmd/ld/lib.c:1864: put(nil, ".args", 'm', 0x80000000, 0, 0, 0);
Introduce a define for this constant.

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

https://codereview.appspot.com/8022044/diff/14001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1356: } else if(gp->gcstack != (uintptr)nil) {
does this need a check if gp->status == Gsyscall ?

https://codereview.appspot.com/8022044/diff/14001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/8022044/diff/14001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:236: func[nfunc-1].args = (sym->value == 0x80000000) ?
-1 : sym->value;
Why not just keep 0x80000000 as the unknown value here?

[cshapiro1, 2013-03-27 20:22:09]

On Wed, Mar 27, 2013 at 9:27 AM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> I'm also unsure whether this can even go in before Go 1.1.  This feels
> scary to me, especially considering we already have a few mystery crashes
> in the runtime (GC/scheduler/netpoll/maps?)  How confident in it are you?
>

I am as confident as I can be from just running the tests over and over
again on all three platforms.  It seems very solid.  Maybe there is some
interal code we can use to evaluate this change?

[cshapiro1, 2013-03-28 01:36:07]

https://codereview.appspot.com/8022044/diff/14001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/8022044/diff/14001/src/cmd/ld/lib.c#newcode1864
src/cmd/ld/lib.c:1864: put(nil, ".args", 'm', 0x80000000, 0, 0, 0);
A good idea.

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

https://codereview.appspot.com/8022044/diff/14001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1356: } else if(gp->gcstack != (uintptr)nil) {
Good question.

It looks like it is mostly true that gcstack is not null when status is
Gsyscall.  When it is not, the code in pkg/runtime/proc.c goes to some effort to
ensure that the clearing of gcstack and returning status to the running state
occurs in just the right order to preserve the invariants needed by the GC.

This case is the same one found in the current version on line 1346 and serves
to tell us where the top of the stack is.  The same information is retrieved and
used in the same way as it was before.  I am pretty confident this is okay.

https://codereview.appspot.com/8022044/diff/14001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/8022044/diff/14001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:236: func[nfunc-1].args = (sym->value == 0x80000000) ?
-1 : sym->value;
Works for me.  Nothing tests for the value -1, only that the value is less than
0.  This should just work.

[cshapiro1, 2013-03-28 01:38:08]

PTAL

My latest patch set addresses Daniel's comments and adds a flag
"ScanStackByFrames" that enables the functionality in this change.  At the
moment the flag is off which means the GC should behave as it always has
scanning every word on the stack.  After we branch for the release turning this
flag on can be explored.

[ality, 2013-03-28 09:42:39]

https://codereview.appspot.com/8022044/diff/22001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/8022044/diff/22001/src/cmd/ld/lib.c#newcode1866
src/cmd/ld/lib.c:1866: if(s->text->textflag & NOSPLIT)
NOSPLIT assembly functions can have s->args > 0
See src/pkg/crypto/sha1/sha1block_amd64.s:/^TEXT

I'm not sure if you can actually distinguish
between a NOSPLIT assembly function without
parameters and one that isn't annotated with
the size of its parameters.

[DMorsing, 2013-03-28 13:46:15]

LGTM

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

https://codereview.appspot.com/8022044/diff/22001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1337: if(f->args > 0)
This needs to be updated to the new constant.

https://codereview.appspot.com/8022044/diff/22001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1391: if (ScanStackByFrames) {
I think the new version can go in directly. It seems like you've tested this
thoroughly.

Does anyone disagree with this?

[iant, 2013-03-28 17:00:54]

On 2013/03/28 13:46:15, DMorsing wrote:
>
> I think the new version can go in directly. It seems like you've tested this
> thoroughly.
> 
> Does anyone disagree with this?

Since there are some outstanding undiagnosed crash bugs, I'm concerned about
stability for the Go 1.1 release.  Certainly I agree that this should be
installed directly after Go 1.1.

(I have not yet looked at this CL, sorry.)

[cshapiro1, 2013-03-28 21:13:53]

The most recent upload should address the remaining review comments.

https://codereview.appspot.com/8022044/diff/22001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/8022044/diff/22001/src/cmd/ld/lib.c#newcode1866
src/cmd/ld/lib.c:1866: if(s->text->textflag & NOSPLIT)
Yes, nosplit functions can be annotated with an argument count.

This code is needed for varargs routines which are marked as nosplit, have
variable arity, and are identified as having an argument size of 0.  Examples
include runtime·convT2I and runtime·convT2E.

Most of the uses of varargs functions can be replaced by calls to equivalent
fixed arity functions.  The remaining varargs calls should not appear in the
backtrace during a garbage collection or can be treated as a special case.

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

https://codereview.appspot.com/8022044/diff/22001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1337: if(f->args > 0)
It should work fine without it but I would agree that it helps the reader to use
the constant.  I'll go ahead and fix that.

[cshapiro, 2013-03-28 21:36:48]

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

cmd/ld, runtime: restrict stack root scan to locals and arguments

Updates issue 5134

R=golang-dev, bradfitz, cshapiro, daniel.morsing, ality, iant
CC=golang-dev
https://codereview.appspot.com/8022044