code review 54650044: runtime: grow stack by copying

runtime: grow stack by copying

On stack overflow, if all frames on the stack are
copyable, we copy the frames to a new stack twice
as large as the old one.  During GC, if a G is using
less than 1/4 of its stack, copy the stack to a stack
half its size.

TODO
- Do something about C frames.  When a C frame is in the
  stack segment, it isn't copyable.  We allocate a new segment
  in this case.
  - For idempotent C code, we can abort it, copy the stack,
    then retry.  I'm working on a separate CL for this.
  - For other C code, we can raise the stackguard
    to the lowest Go frame so the next call that Go frame
    makes triggers a copy, which will then succeed.
- Pick a starting stack size?

The plan is that eventually we reach a point where the
stack contains only copyable frames.

[dvyukov, 2014-02-15 06:43:09]

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

https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:227: // +------------------+ <- frame->argp
This is useful.
Please make it clear whether argp (varp/sp) points to the first word of "args
from caller" or to the "return address". Obviously it cannot point in between
words :)

https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:357: if(t != nil && (t->size > PtrSize || (t->kind &
KindNoPointers) == 0)) {
Can we have a pointer into stack if t->size > PtrSize?
I thought we always allocate from heap in this case.

https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:655: stk = runtime·stackalloc(framesize);
we need to figure out what to do with heap profiler/memstats for stack memory
non-standard stack segments are not accounted against StackSys nor they are heap
profiled
I expect that with growable stacks we will see more non-standard segments (only
allocate with mallocgc eventually?)
they must be accounted somewhere in some way
if you don't resolve it in this CL, please file an issue for Go1.3

https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:751: if(gp->syscallstack != (uintptr)nil) // TODO:
handle this case?
please search for "libcallsp" that I've recently added, you may need to check it
here as well (or maybe not)

[khr, 2014-02-23 08:57:08]

On 2014/02/15 06:43:09, dvyukov wrote:
> https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c
> File src/pkg/runtime/stack.c (right):
> 
>
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:227: // +------------------+ <- frame->argp
> This is useful.
> Please make it clear whether argp (varp/sp) points to the first word of "args
> from caller" or to the "return address". Obviously it cannot point in between
> words :)

Argp does point "in between".  It marks the separation point between the return
address and the args.  I'd be happy to make this clearer but I'm not sure what
about
it is confusing.

> 
>
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:357: if(t != nil && (t->size > PtrSize || (t->kind &
> KindNoPointers) == 0)) {
> Can we have a pointer into stack if t->size > PtrSize?
> I thought we always allocate from heap in this case.
> 

I don't think we currently ever stack-allocate in this case.  But it is possible
that we might do so some day.

>
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:655: stk = runtime·stackalloc(framesize);
> we need to figure out what to do with heap profiler/memstats for stack memory
> non-standard stack segments are not accounted against StackSys nor they are
heap
> profiled
> I expect that with growable stacks we will see more non-standard segments
(only
> allocate with mallocgc eventually?)
> they must be accounted somewhere in some way
> if you don't resolve it in this CL, please file an issue for Go1.3
> 

Will do.

>
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:751: if(gp->syscallstack != (uintptr)nil) // TODO:
> handle this case?
> please search for "libcallsp" that I've recently added, you may need to check
it
> here as well (or maybe not)

I've added it as well, just in case.

[khr, 2014-02-23 08:57:48]

Hello dvyukov@google.com (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://khr%40golang.org@code.google.com/p/go/

[dvyukov, 2014-02-23 10:21:04]

On 2014/02/23 08:57:08, khr wrote:
> On 2014/02/15 06:43:09, dvyukov wrote:
> > https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c
> > File src/pkg/runtime/stack.c (right):
> > 
> >
>
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
> > src/pkg/runtime/stack.c:227: // +------------------+ <- frame->argp
> > This is useful.
> > Please make it clear whether argp (varp/sp) points to the first word of
"args
> > from caller" or to the "return address". Obviously it cannot point in
between
> > words :)
> 
> Argp does point "in between".  It marks the separation point between the
return
> address and the args.  I'd be happy to make this clearer but I'm not sure what
> about
> it is confusing.

Pointers can't point in between words. A pointer can point either at (1) the
first word of argp or at (2) the ret addr or (3) at something in between (if
it's the case please add that something to the picture).
The picture is confusing because it does not allow to me to answer the question
-- what I get if I dereference the pointer.
I mean something along the lines of:


// +------------------+
// |  return address  |
// +------------------+ 
// |                          |  <- frame->varp
// |     locals       |
// |                          |
// +------------------+

[dvyukov, 2014-02-23 10:30:44]

>
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
> > src/pkg/runtime/stack.c:357: if(t != nil && (t->size > PtrSize || (t->kind &
> > KindNoPointers) == 0)) {
> > Can we have a pointer into stack if t->size > PtrSize?
> > I thought we always allocate from heap in this case.
> > 
> 
> I don't think we currently ever stack-allocate in this case.  But it is
possible
> that we might do so some day.

Then I think it's better to do (for both sanity checking and code
documentation):

if(t != nil && (t->size > PtrSize || (t->kind & KindNoPointers) == 0)) {
    p = scanp[i+1];
    if((uintptr)p >= (uintptr)minp && (uintptr)p < (uintptr)maxp) {
        if(t->size > PtrSize)  // currently we always allocate such objects on
heap
            runtime.throw("...");
        ...
    }
}

And the same for Iface.

Is escape analysis good enough to allow !KindNoPointers interfaces to be on
stack?

[dvyukov, 2014-02-23 13:19:52]

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

https://codereview.appspot.com/54650044/diff/240001/src/pkg/runtime/mgc0.c#ne...
src/pkg/runtime/mgc0.c:1614: runtime·shrinkstack(gp);
I would put this after "runtime·throw("can't scan gchelper stack");" line
and let shrinkstack itself decide when it can/want shrink

https://codereview.appspot.com/54650044/diff/240001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/54650044/diff/240001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:176: // TODO: temporary
how temporal is this?
it must not survive release point, right?

I think we just need to do:
runtime·copystack = true;

it's enough for testing/debugging/experimentation and as an emergency switch.

https://codereview.appspot.com/54650044/diff/240001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:198: // is stored in a Defer, not in the frame itself.
I do not understand this.
Heap has a pointer into runtime.main stack and we do not update pointers in heap
during stack growth.
It looks to me that runtime.main frame is not movable in its current state.

If my understanding is correct, then:
1. Add a test that grows main stack and executes the defer (and crashes now).
2. Make the defer heap-allocated or global.

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

https://codereview.appspot.com/54650044/diff/240001/src/pkg/runtime/traceback...
src/pkg/runtime/traceback_x86.c:156: n = max - 1;
it's somewhat non-obvious that this means "exit now", and this function is messy
enough for this to break in future.
just return n?

[dvyukov, 2014-02-23 13:38:48]

do add new tests
I am sure that all.bash does some growing/shrinking, but it's unclear whether it
tests all interesting cases or not. It's also useful to have a comprehensive
test for a single aspect when debugging. The set of weird tests in runtime
package is a very important and useful asset.

I can think of at least 2 interesting sets of tests:
1. write a [recursive] function that grows stack, and call it from init, main
goroutine, helper goroutine, finalizer, LockOSThread goroutine, cgo callback.
2. write a function that accepts another function f and calls it with varying
amount of stack space available (say with StackMin-1024 to StackMin with step 8
stack space consumed); and call it with different f functions (chan ops, map
ops, malloc, goroutine creation, etc).

Insert runtime.GC into 1 and 2 somewhere to test shrinkstack.

Test a case when goroutine finishes with large stack.

[dvyukov, 2014-02-25 13:46:58]

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

https://codereview.appspot.com/54650044/diff/240001/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:103: // Stacks are usually allocated with a fixed-size
free-list allocator,
please update this comment

[rsc, 2014-02-25 19:49:42]

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:198: // is stored in a Defer, not in the frame itself.
I don't understand this. If the frame is moved, won't the copy stored in the
Defer be wrong?
Or is the Defer on some other part of the stack too?

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c
File src/pkg/runtime/stack.c (left):

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#o...
src/pkg/runtime/stack.c:93: if(n != FixedStack) {
I think this should stay. The stackcache only contains FixedStack-sized chunks,
so if n != FixedStack, there's a problem here.

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

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:255: if((uintptr)frame->varp < (uintptr)cinfo->stk ||
(uintptr)frame->varp >= (uintptr)cinfo->base) {
seems like these (uintptr) casts can all go away. everything here is a byte*.

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:267: if(frame->varp != (byte*)frame->sp) { // not in
prologue (or no locals&outargs)
i am not sure what the comment is trying to say by "(or no locals&outargs)".
i think perhaps it is reversed and you mean (and NOT no locals&outargs).
better might be:

// stack frame has been allocated

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:315: byte *oldbase;	// top address of segment (including
Stktop)
s/including/after/
(or before but i think you mean after)

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:316: uintptr delta;  // ptr distance from old to new
stack
add " (newstk - oldstk)"

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:348: if((uintptr)p >= (uintptr)minp && (uintptr)p <
(uintptr)maxp) {
tiny nit but we've been trying to consistently write
    lo <= p && p < hi
so that it looks more like what you'd say in non-programming: lo <= p < hi.
similarly
    p < lo || hi <= p

also, seems like the casts can all go away, which will help make it easier to
scan.

same for the other instances below
(and actually, also the one above comparing against nil)

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:415: // adjust inargs
// adjust inargs and outargs

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:423: // TODO: set last arg if pointermap is not from C
code.
what does this mean?

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:432: if((uintptr)gp->sched.ctxt >=
(uintptr)adjinfo->oldstk && (uintptr)gp->sched.ctxt < (uintptr)adjinfo->oldbase)
if you use (byte*)gp->sched.ctxt here you can drop half the casts

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:446: if((uintptr)d >= (uintptr)adjinfo->oldstk &&
(uintptr)d < (uintptr)adjinfo->oldbase) {
if you use (byte*)d here then you can drop half the casts
also in various places below

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:458: runtime·printf("bad defer %p %d %d %p %p\n",
d->fn->fn, d->siz, d->special, d->argp, d->pc);
we're not completely consistent but i've been trying to make crashes start with
"runtime: "
so that it is a little clearer that the message didn't come from the user's
program.
so s/"/"runtime: / here, and then you've said it and don't need to worry about
it for the next print.

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:619: // and make a copy instead of allocating a new
segment.
s/make a copy/move the segment/

"make a copy" sounds like you end up with two

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:643: if(true) {
remove + unindent body

[rsc, 2014-02-25 20:26:20]

I'd like to try to get this in today or tomorrow. We can add more tests later
(and debug build breakages).

[khr, 2014-02-26 22:17:11]

On 2014/02/23 10:21:04, dvyukov wrote:
> On 2014/02/23 08:57:08, khr wrote:
> > On 2014/02/15 06:43:09, dvyukov wrote:
> > >
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c
> > > File src/pkg/runtime/stack.c (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
> > > src/pkg/runtime/stack.c:227: // +------------------+ <- frame->argp
> > > This is useful.
> > > Please make it clear whether argp (varp/sp) points to the first word of
> "args
> > > from caller" or to the "return address". Obviously it cannot point in
> between
> > > words :)
> > 
> > Argp does point "in between".  It marks the separation point between the
> return
> > address and the args.  I'd be happy to make this clearer but I'm not sure
what
> > about
> > it is confusing.
> 
> Pointers can't point in between words. A pointer can point either at (1) the
> first word of argp or at (2) the ret addr or (3) at something in between (if
> it's the case please add that something to the picture).
> The picture is confusing because it does not allow to me to answer the
question
> -- what I get if I dereference the pointer.
> I mean something along the lines of:
> 
> 
> // +------------------+
> // |  return address  |
> // +------------------+ 
> // |                          |  <- frame->varp
> // |     locals       |
> // |                          |
> // +------------------+

I'd disagree with you here.  The pointers involved (argp,varp,sp) don't
point at particular items in memory.  They demarcate ranges of memory.  You
never
dereference them directly; instead you compare them, like sp <= p && p < varp.

[khr, 2014-02-26 22:43:17]

On 2014/02/23 10:30:44, dvyukov wrote:
> >
>
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
> > > src/pkg/runtime/stack.c:357: if(t != nil && (t->size > PtrSize || (t->kind
&
> > > KindNoPointers) == 0)) {
> > > Can we have a pointer into stack if t->size > PtrSize?
> > > I thought we always allocate from heap in this case.
> > > 
> > 
> > I don't think we currently ever stack-allocate in this case.  But it is
> possible
> > that we might do so some day.
> 
> Then I think it's better to do (for both sanity checking and code
> documentation):
> 
> if(t != nil && (t->size > PtrSize || (t->kind & KindNoPointers) == 0)) {
>     p = scanp[i+1];
>     if((uintptr)p >= (uintptr)minp && (uintptr)p < (uintptr)maxp) {
>         if(t->size > PtrSize)  // currently we always allocate such objects on
> heap
>             runtime.throw("...");
>         ...
>     }
> }
> 
> And the same for Iface.

Done.

> 
> Is escape analysis good enough to allow !KindNoPointers interfaces to be on
> stack?

Yes, interfaces whose concrete type is a pointer can be allocated on the stack.

[khr, 2014-02-26 22:48:33]

On 2014/02/23 13:19:52, dvyukov wrote:
> https://codereview.appspot.com/54650044/diff/240001/src/pkg/runtime/mgc0.c
> File src/pkg/runtime/mgc0.c (right):
> 
>
https://codereview.appspot.com/54650044/diff/240001/src/pkg/runtime/mgc0.c#ne...
> src/pkg/runtime/mgc0.c:1614: runtime·shrinkstack(gp);
> I would put this after "runtime·throw("can't scan gchelper stack");" line
> and let shrinkstack itself decide when it can/want shrink
> 

Done.

> https://codereview.appspot.com/54650044/diff/240001/src/pkg/runtime/proc.c
> File src/pkg/runtime/proc.c (right):
> 
>
https://codereview.appspot.com/54650044/diff/240001/src/pkg/runtime/proc.c#ne...
> src/pkg/runtime/proc.c:176: // TODO: temporary
> how temporal is this?
> it must not survive release point, right?
> 
> I think we just need to do:
> runtime·copystack = true;
> 
> it's enough for testing/debugging/experimentation and as an emergency switch.
> 

I inverted the sense (copying stack is the default, GOCOPYSTACK=0 now disables
it).
It is initialized to runtime·precisestack, as it won't work without precise
stack info.

>
https://codereview.appspot.com/54650044/diff/240001/src/pkg/runtime/proc.c#ne...
> src/pkg/runtime/proc.c:198: // is stored in a Defer, not in the frame itself.
> I do not understand this.
> Heap has a pointer into runtime.main stack and we do not update pointers in
heap
> during stack growth.
> It looks to me that runtime.main frame is not movable in its current state.
> 
> If my understanding is correct, then:
> 1. Add a test that grows main stack and executes the defer (and crashes now).
> 2. Make the defer heap-allocated or global.
> 

We do update Defer pointers, including pointers coming from the heap.  So this
Defer is copyable because the only pointer to it is in another deeper
defer or in G.defer.

>
https://codereview.appspot.com/54650044/diff/240001/src/pkg/runtime/traceback...
> File src/pkg/runtime/traceback_x86.c (right):
> 
>
https://codereview.appspot.com/54650044/diff/240001/src/pkg/runtime/traceback...
> src/pkg/runtime/traceback_x86.c:156: n = max - 1;
> it's somewhat non-obvious that this means "exit now", and this function is
messy
> enough for this to break in future.
> just return n?

Sure.

[khr, 2014-02-26 22:49:13]

On 2014/02/23 13:38:48, dvyukov wrote:
> do add new tests
> I am sure that all.bash does some growing/shrinking, but it's unclear whether
it
> tests all interesting cases or not. It's also useful to have a comprehensive
> test for a single aspect when debugging. The set of weird tests in runtime
> package is a very important and useful asset.
> 
> I can think of at least 2 interesting sets of tests:
> 1. write a [recursive] function that grows stack, and call it from init, main
> goroutine, helper goroutine, finalizer, LockOSThread goroutine, cgo callback.
> 2. write a function that accepts another function f and calls it with varying
> amount of stack space available (say with StackMin-1024 to StackMin with step
8
> stack space consumed); and call it with different f functions (chan ops, map
> ops, malloc, goroutine creation, etc).
> 
> Insert runtime.GC into 1 and 2 somewhere to test shrinkstack.
> 
> Test a case when goroutine finishes with large stack.

I will add as a separate CL.

[khr, 2014-02-26 22:49:34]

On 2014/02/25 13:46:58, dvyukov wrote:
> https://codereview.appspot.com/54650044/diff/240001/src/pkg/runtime/stack.c
> File src/pkg/runtime/stack.c (right):
> 
>
https://codereview.appspot.com/54650044/diff/240001/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:103: // Stacks are usually allocated with a fixed-size
> free-list allocator,
> please update this comment

Done.

[khr, 2014-02-26 22:55:33]

On 2014/02/25 19:49:42, rsc wrote:
> https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/proc.c
> File src/pkg/runtime/proc.c (right):
> 
>
https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/proc.c#ne...
> src/pkg/runtime/proc.c:198: // is stored in a Defer, not in the frame itself.
> I don't understand this. If the frame is moved, won't the copy stored in the
> Defer be wrong?
> Or is the Defer on some other part of the stack too?

What copy stored in the Defer?
See stack.c:adjustdefer, it has code to handle the fact that a Defer might be on
the stack.

> https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c
> File src/pkg/runtime/stack.c (left):
> 
>
https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#o...
> src/pkg/runtime/stack.c:93: if(n != FixedStack) {
> I think this should stay. The stackcache only contains FixedStack-sized
chunks,
> so if n != FixedStack, there's a problem here.
> 

Restored.

> https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c
> File src/pkg/runtime/stack.c (right):
> 
>
https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:255: if((uintptr)frame->varp < (uintptr)cinfo->stk ||
> (uintptr)frame->varp >= (uintptr)cinfo->base) {
> seems like these (uintptr) casts can all go away. everything here is a byte*.
> 

Fixed.

>
https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:267: if(frame->varp != (byte*)frame->sp) { // not in
> prologue (or no locals&outargs)
> i am not sure what the comment is trying to say by "(or no locals&outargs)".
> i think perhaps it is reversed and you mean (and NOT no locals&outargs).
> better might be:
> 
> // stack frame has been allocated
> 

I changed the wording to be correct, and hopefully clearer.
"not in prologue (and has at least one local or outarg)"

>
https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:315: byte *oldbase;	// top address of segment
(including
> Stktop)
> s/including/after/
> (or before but i think you mean after)
> 

Fixed.

>
https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:316: uintptr delta;  // ptr distance from old to new
> stack
> add " (newstk - oldstk)"

It's newbase - oldbase.  Added.

> 
>
https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:348: if((uintptr)p >= (uintptr)minp && (uintptr)p <
> (uintptr)maxp) {
> tiny nit but we've been trying to consistently write
>     lo <= p && p < hi
> so that it looks more like what you'd say in non-programming: lo <= p < hi.
> similarly
>     p < lo || hi <= p
> 
> also, seems like the casts can all go away, which will help make it easier to
> scan.
> 
> same for the other instances below
> (and actually, also the one above comparing against nil)

I think I found and fixed all of these.

>
https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:415: // adjust inargs
> // adjust inargs and outargs
> 

Fixed.

>
https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:423: // TODO: set last arg if pointermap is not from C
> code.
> what does this mean?
> 

I don't know.  Ancient cruft.  Removed.

>
https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:432: if((uintptr)gp->sched.ctxt >=
> (uintptr)adjinfo->oldstk && (uintptr)gp->sched.ctxt <
(uintptr)adjinfo->oldbase)
> if you use (byte*)gp->sched.ctxt here you can drop half the casts
> 
>
https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:446: if((uintptr)d >= (uintptr)adjinfo->oldstk &&
> (uintptr)d < (uintptr)adjinfo->oldbase) {
> if you use (byte*)d here then you can drop half the casts
> also in various places below

Done.

> 
>
https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:458: runtime·printf("bad defer %p %d %d %p %p\n",
> d->fn->fn, d->siz, d->special, d->argp, d->pc);
> we're not completely consistent but i've been trying to make crashes start
with
> "runtime: "
> so that it is a little clearer that the message didn't come from the user's
> program.
> so s/"/"runtime: / here, and then you've said it and don't need to worry about
> it for the next print.
> 

Added.

>
https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:619: // and make a copy instead of allocating a new
> segment.
> s/make a copy/move the segment/
> 
> "make a copy" sounds like you end up with two
> 

Fixed.

>
https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/stack.c#n...
> src/pkg/runtime/stack.c:643: if(true) {
> remove + unindent body

Fixed.

[rsc, 2014-02-27 02:35:52]

LGTM

Please submit when you're ready.

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

https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
src/pkg/runtime/stack.c:357: if(t != nil && (t->size > PtrSize || (t->kind &
KindNoPointers) == 0)) {
On 2014/02/15 06:43:09, dvyukov wrote:
> Can we have a pointer into stack if t->size > PtrSize?
> I thought we always allocate from heap in this case.

I don't think so, but might as well handle it anyway.

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/54650044/diff/260001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:198: // is stored in a Defer, not in the frame itself.
Okay, I think I understand what confused me. There are actually two separate
concerns. I was worried, because this is C, that we don't know the precise
layout of the frame, and in particular we don't know which words inside the
Defer are pointers and need adjusting. The other concern is: because this is C,
where might it have passed pointers into the frame? The comment is addressing
the latter and I thought it was addressing the former. I suggest a longer
comment:

// Note: C frames in general are not copyable during stack growth, for two
reasons:
//
//   1) We don't know where in a frame to find pointers to other stack
locations.
//   2) There's no guarantee that globals or heap values do not point into the
frame.
//
// The C frame for runtime.main is copyable, because:
//
//   1) There are no pointers to other stack locations in the frame
//      (d.fn points at a global, d.link is nil, d.argp is -1).
//   2) The only pointer into this frame is from the defer chain,
//      which is explicitly handled during stack copying.

[khr, 2014-02-27 07:28:47]

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

runtime: grow stack by copying

On stack overflow, if all frames on the stack are
copyable, we copy the frames to a new stack twice
as large as the old one.  During GC, if a G is using
less than 1/4 of its stack, copy the stack to a stack
half its size.

TODO
- Do something about C frames.  When a C frame is in the
  stack segment, it isn't copyable.  We allocate a new segment
  in this case.
  - For idempotent C code, we can abort it, copy the stack,
    then retry.  I'm working on a separate CL for this.
  - For other C code, we can raise the stackguard
    to the lowest Go frame so the next call that Go frame
    makes triggers a copy, which will then succeed.
- Pick a starting stack size?

The plan is that eventually we reach a point where the
stack contains only copyable frames.

LGTM=rsc
R=dvyukov, rsc
CC=golang-codereviews
https://codereview.appspot.com/54650044

[gobot, 2014-02-27 07:32:03]

This CL appears to have broken the solaris-amd64-solaris11 builder.

[dave_cheney.net, 2014-02-27 07:38:17]

fatal error: sweep of incache span

goroutine 41 [running]:
runtime.throw(0x7a1dba)
	/home/dfc/workspace/solaris-amd64-solaris11-125e69d99b5d/go/src/pkg/runtime/panic.c:463
+0x69 fp=0xc208220718


This is on 64bit hardware. The previous odd failures we've seen have
been under 8g.



On Thu, Feb 27, 2014 at 6:32 PM, <gobot@golang.org> wrote:

> This CL appears to have broken the solaris-amd64-solaris11 builder.
>
>
> https://codereview.appspot.com/54650044/
>
> --
> 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/groups/opt_out.
>

[bradfitz, 2014-02-27 08:15:38]

Looks like the whole world's broken, except oddly the linux-race builder.



On Wed, Feb 26, 2014 at 11:38 PM, Dave Cheney <dave@cheney.net> wrote:

> fatal error: sweep of incache span
>
> goroutine 41 [running]:
> runtime.throw(0x7a1dba)
>
	/home/dfc/workspace/solaris-amd64-solaris11-125e69d99b5d/go/src/pkg/runtime/panic.c:463
+0x69 fp=0xc208220718
>
>
> This is on 64bit hardware. The previous odd failures we've seen have been
under 8g.
>
>
>
> On Thu, Feb 27, 2014 at 6:32 PM, <gobot@golang.org> wrote:
>
>> This CL appears to have broken the solaris-amd64-solaris11 builder.
>>
>>
>> https://codereview.appspot.com/54650044/
>>
>> --
>> 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/groups/opt_out.
>>
>
>  --
> 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/groups/opt_out.
>

[dave_cheney.net, 2014-02-27 08:36:38]

oh dear, so it is. Go linux-race builder, you can do it!


On Thu, Feb 27, 2014 at 7:15 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Looks like the whole world's broken, except oddly the linux-race builder.
>
>
>
> On Wed, Feb 26, 2014 at 11:38 PM, Dave Cheney <dave@cheney.net> wrote:
>
>> fatal error: sweep of incache span
>>
>> goroutine 41 [running]:
>> runtime.throw(0x7a1dba)
>>
	/home/dfc/workspace/solaris-amd64-solaris11-125e69d99b5d/go/src/pkg/runtime/panic.c:463
+0x69 fp=0xc208220718
>>
>>
>> This is on 64bit hardware. The previous odd failures we've seen have been
under 8g.
>>
>>
>>
>> On Thu, Feb 27, 2014 at 6:32 PM, <gobot@golang.org> wrote:
>>
>>> This CL appears to have broken the solaris-amd64-solaris11 builder.
>>>
>>>
>>> https://codereview.appspot.com/54650044/
>>>
>>> --
>>> 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/groups/opt_out.
>>>
>>
>>  --
>> 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/groups/opt_out.
>>
>
>

[bsiegert, 2014-02-27 09:28:57]

Undo maybe?

[khr1, 2014-02-27 09:43:22]

Some facts in issue 7420.  I'll revert for the moment, going to bed...



On Thu, Feb 27, 2014 at 1:28 AM, <bsiegert@gmail.com> wrote:

> Undo maybe?
>
> https://codereview.appspot.com/54650044/
>

[dvyukov, 2014-02-27 16:05:17]

On Thu, Feb 27, 2014 at 2:17 AM,  <khr@golang.org> wrote:
> On 2014/02/23 10:21:04, dvyukov wrote:
>>
>> On 2014/02/23 08:57:08, khr wrote:
>> > On 2014/02/15 06:43:09, dvyukov wrote:
>> > >
>
> https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c
>>
>> > > File src/pkg/runtime/stack.c (right):
>> > >
>> > >
>> >
>
>
>
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
>>
>> > > src/pkg/runtime/stack.c:227: // +------------------+ <-
>
> frame->argp
>>
>> > > This is useful.
>> > > Please make it clear whether argp (varp/sp) points to the first
>
> word of
>>
>> "args
>> > > from caller" or to the "return address". Obviously it cannot point
>
> in
>>
>> between
>> > > words :)
>> >
>> > Argp does point "in between".  It marks the separation point between
>
> the
>>
>> return
>> > address and the args.  I'd be happy to make this clearer but I'm not
>
> sure what
>>
>> > about
>> > it is confusing.
>
>
>> Pointers can't point in between words. A pointer can point either at
>
> (1) the
>>
>> first word of argp or at (2) the ret addr or (3) at something in
>
> between (if
>>
>> it's the case please add that something to the picture).
>> The picture is confusing because it does not allow to me to answer the
>
> question
>>
>> -- what I get if I dereference the pointer.
>> I mean something along the lines of:
>
>
>
>> // +------------------+
>> // |  return address  |
>> // +------------------+
>> // |                          |  <- frame->varp
>> // |     locals       |
>> // |                          |
>> // +------------------+
>
>
> I'd disagree with you here.  The pointers involved (argp,varp,sp) don't
> point at particular items in memory.  They demarcate ranges of memory.
> You never
> dereference them directly; instead you compare them, like sp <= p && p <
> varp.


Is anything of the following true?
1. varp points to the first word of local variables?
2. varp points to the word right before first word of local variables?
3. varp points to the last word of local variables?
4. varp points to the word right after local variables?

I can't understand how it can point nowhere/between words. When we
scan frames in GC we need to know whether we need to dereference varp,
varp+1 or varp-1.

[khr1, 2014-02-27 16:14:31]

#4.

Local variables (and args to callee) are in all addresses p such that
frame->sp <= p < frame->varp.

Return address info is in all addresses p such that frame->varp <= p <
frame->argp.

Args from the caller are in addresses p with frame->argp <= p.


On Thu, Feb 27, 2014 at 8:04 AM, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Thu, Feb 27, 2014 at 2:17 AM,  <khr@golang.org> wrote:
> > On 2014/02/23 10:21:04, dvyukov wrote:
> >>
> >> On 2014/02/23 08:57:08, khr wrote:
> >> > On 2014/02/15 06:43:09, dvyukov wrote:
> >> > >
> >
> >
> https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c
> >>
> >> > > File src/pkg/runtime/stack.c (right):
> >> > >
> >> > >
> >> >
> >
> >
> >
>
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
> >>
> >> > > src/pkg/runtime/stack.c:227: // +------------------+ <-
> >
> > frame->argp
> >>
> >> > > This is useful.
> >> > > Please make it clear whether argp (varp/sp) points to the first
> >
> > word of
> >>
> >> "args
> >> > > from caller" or to the "return address". Obviously it cannot point
> >
> > in
> >>
> >> between
> >> > > words :)
> >> >
> >> > Argp does point "in between".  It marks the separation point between
> >
> > the
> >>
> >> return
> >> > address and the args.  I'd be happy to make this clearer but I'm not
> >
> > sure what
> >>
> >> > about
> >> > it is confusing.
> >
> >
> >> Pointers can't point in between words. A pointer can point either at
> >
> > (1) the
> >>
> >> first word of argp or at (2) the ret addr or (3) at something in
> >
> > between (if
> >>
> >> it's the case please add that something to the picture).
> >> The picture is confusing because it does not allow to me to answer the
> >
> > question
> >>
> >> -- what I get if I dereference the pointer.
> >> I mean something along the lines of:
> >
> >
> >
> >> // +------------------+
> >> // |  return address  |
> >> // +------------------+
> >> // |                          |  <- frame->varp
> >> // |     locals       |
> >> // |                          |
> >> // +------------------+
> >
> >
> > I'd disagree with you here.  The pointers involved (argp,varp,sp) don't
> > point at particular items in memory.  They demarcate ranges of memory.
> > You never
> > dereference them directly; instead you compare them, like sp <= p && p <
> > varp.
>
>
> Is anything of the following true?
> 1. varp points to the first word of local variables?
> 2. varp points to the word right before first word of local variables?
> 3. varp points to the last word of local variables?
> 4. varp points to the word right after local variables?
>
> I can't understand how it can point nowhere/between words. When we
> scan frames in GC we need to know whether we need to dereference varp,
> varp+1 or varp-1.
>

[dvyukov, 2014-03-04 06:20:19]

On 2014/02/23 08:57:08, khr wrote:
> On 2014/02/15 06:43:09, dvyukov wrote:
> > https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c
> > File src/pkg/runtime/stack.c (right):
> > 
> >
>
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
> > src/pkg/runtime/stack.c:227: // +------------------+ <- frame->argp
> > This is useful.
> > Please make it clear whether argp (varp/sp) points to the first word of
"args
> > from caller" or to the "return address". Obviously it cannot point in
between
> > words :)
> 
> Argp does point "in between".  It marks the separation point between the
return
> address and the args.  I'd be happy to make this clearer but I'm not sure what
> about
> it is confusing.
> 
> > 
> >
>
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
> > src/pkg/runtime/stack.c:357: if(t != nil && (t->size > PtrSize || (t->kind &
> > KindNoPointers) == 0)) {
> > Can we have a pointer into stack if t->size > PtrSize?
> > I thought we always allocate from heap in this case.
> > 
> 
> I don't think we currently ever stack-allocate in this case.  But it is
possible
> that we might do so some day.
> 
> >
>
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
> > src/pkg/runtime/stack.c:655: stk = runtime·stackalloc(framesize);
> > we need to figure out what to do with heap profiler/memstats for stack
memory
> > non-standard stack segments are not accounted against StackSys nor they are
> heap
> > profiled
> > I expect that with growable stacks we will see more non-standard segments
> (only
> > allocate with mallocgc eventually?)
> > they must be accounted somewhere in some way
> > if you don't resolve it in this CL, please file an issue for Go1.3
> > 
> 
> Will do.

please do


>
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
> > src/pkg/runtime/stack.c:751: if(gp->syscallstack != (uintptr)nil) // TODO:
> > handle this case?
> > please search for "libcallsp" that I've recently added, you may need to
check
> it
> > here as well (or maybe not)
> 
> I've added it as well, just in case.

I do not see libcallsp in source code, where did you add it?

[khr1, 2014-03-04 21:51:49]

On Mon, Mar 3, 2014 at 10:20 PM, <dvyukov@google.com> wrote:

> On 2014/02/23 08:57:08, khr wrote:
>
>> On 2014/02/15 06:43:09, dvyukov wrote:
>> >
>>
> https://codereview.appspot.com/54650044/diff/140002/src/
> pkg/runtime/stack.c
>
>> > File src/pkg/runtime/stack.c (right):
>> >
>> >
>>
>
> https://codereview.appspot.com/54650044/diff/140002/src/
> pkg/runtime/stack.c#newcode227
>
>> > src/pkg/runtime/stack.c:227: // +------------------+ <- frame->argp
>> > This is useful.
>> > Please make it clear whether argp (varp/sp) points to the first word
>>
> of "args
>
>> > from caller" or to the "return address". Obviously it cannot point
>>
> in between
>
>> > words :)
>>
>
>  Argp does point "in between".  It marks the separation point between
>>
> the return
>
>> address and the args.  I'd be happy to make this clearer but I'm not
>>
> sure what
>
>> about
>> it is confusing.
>>
>
>  >
>> >
>>
>
> https://codereview.appspot.com/54650044/diff/140002/src/
> pkg/runtime/stack.c#newcode357
>
>> > src/pkg/runtime/stack.c:357: if(t != nil && (t->size > PtrSize ||
>>
> (t->kind &
>
>> > KindNoPointers) == 0)) {
>>
>> > Can we have a pointer into stack if t->size > PtrSize?
>> > I thought we always allocate from heap in this case.
>> >
>>
>
>  I don't think we currently ever stack-allocate in this case.  But it
>>
> is possible
>
>> that we might do so some day.
>>
>
>  >
>>
>
> https://codereview.appspot.com/54650044/diff/140002/src/
> pkg/runtime/stack.c#newcode655
>
>> > src/pkg/runtime/stack.c:655: stk = runtime·stackalloc(framesize);
>> > we need to figure out what to do with heap profiler/memstats for
>>
> stack memory
>
>> > non-standard stack segments are not accounted against StackSys nor
>>
> they are
>
>> heap
>> > profiled
>> > I expect that with growable stacks we will see more non-standard
>>
> segments
>
>> (only
>> > allocate with mallocgc eventually?)
>> > they must be accounted somewhere in some way
>> > if you don't resolve it in this CL, please file an issue for Go1.3
>> >
>>
>
>  Will do.
>>
>
> please do
>
>
# 7468


>
>
> https://codereview.appspot.com/54650044/diff/140002/src/
> pkg/runtime/stack.c#newcode751
>
>> > src/pkg/runtime/stack.c:751: if(gp->syscallstack != (uintptr)nil) //
>>
> TODO:
>
>> > handle this case?
>> > please search for "libcallsp" that I've recently added, you may need
>>
> to check
>
>> it
>> > here as well (or maybe not)
>>
>
>  I've added it as well, just in case.
>>
>
> I do not see libcallsp in source code, where did you add it?
>
>
Hmmm, I added it but it appears to have vanished.  Probably I lost it
during a sync.  CL coming.


>
> https://codereview.appspot.com/54650044/
>

[dvyukov, 2014-05-27 17:55:53]

mailed https://codereview.appspot.com/98640043

On Thu, Feb 27, 2014 at 8:14 PM, Keith Randall <khr@google.com> wrote:
> #4.
>
> Local variables (and args to callee) are in all addresses p such that
> frame->sp <= p < frame->varp.
>
> Return address info is in all addresses p such that frame->varp <= p <
> frame->argp.
>
> Args from the caller are in addresses p with frame->argp <= p.
>
>
> On Thu, Feb 27, 2014 at 8:04 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> On Thu, Feb 27, 2014 at 2:17 AM,  <khr@golang.org> wrote:
>> > On 2014/02/23 10:21:04, dvyukov wrote:
>> >>
>> >> On 2014/02/23 08:57:08, khr wrote:
>> >> > On 2014/02/15 06:43:09, dvyukov wrote:
>> >> > >
>> >
>> >
>> >
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c
>> >>
>> >> > > File src/pkg/runtime/stack.c (right):
>> >> > >
>> >> > >
>> >> >
>> >
>> >
>> >
>> >
https://codereview.appspot.com/54650044/diff/140002/src/pkg/runtime/stack.c#n...
>> >>
>> >> > > src/pkg/runtime/stack.c:227: // +------------------+ <-
>> >
>> > frame->argp
>> >>
>> >> > > This is useful.
>> >> > > Please make it clear whether argp (varp/sp) points to the first
>> >
>> > word of
>> >>
>> >> "args
>> >> > > from caller" or to the "return address". Obviously it cannot point
>> >
>> > in
>> >>
>> >> between
>> >> > > words :)
>> >> >
>> >> > Argp does point "in between".  It marks the separation point between
>> >
>> > the
>> >>
>> >> return
>> >> > address and the args.  I'd be happy to make this clearer but I'm not
>> >
>> > sure what
>> >>
>> >> > about
>> >> > it is confusing.
>> >
>> >
>> >> Pointers can't point in between words. A pointer can point either at
>> >
>> > (1) the
>> >>
>> >> first word of argp or at (2) the ret addr or (3) at something in
>> >
>> > between (if
>> >>
>> >> it's the case please add that something to the picture).
>> >> The picture is confusing because it does not allow to me to answer the
>> >
>> > question
>> >>
>> >> -- what I get if I dereference the pointer.
>> >> I mean something along the lines of:
>> >
>> >
>> >
>> >> // +------------------+
>> >> // |  return address  |
>> >> // +------------------+
>> >> // |                          |  <- frame->varp
>> >> // |     locals       |
>> >> // |                          |
>> >> // +------------------+
>> >
>> >
>> > I'd disagree with you here.  The pointers involved (argp,varp,sp) don't
>> > point at particular items in memory.  They demarcate ranges of memory.
>> > You never
>> > dereference them directly; instead you compare them, like sp <= p && p <
>> > varp.
>>
>>
>> Is anything of the following true?
>> 1. varp points to the first word of local variables?
>> 2. varp points to the word right before first word of local variables?
>> 3. varp points to the last word of local variables?
>> 4. varp points to the word right after local variables?
>>
>> I can't understand how it can point nowhere/between words. When we
>> scan frames in GC we need to know whether we need to dereference varp,
>> varp+1 or varp-1.
>
>