code review 137410043: runtime: assume precisestack, copystack, StackCopyAlway...

runtime: assume precisestack, copystack, StackCopyAlways, ScanStackByFrames

Commit to stack copying for stack growth.

We're carrying around a surprising amount of cruft from older schemes.
I am confident that precise stack scans and stack copying are here to stay.

Delete fallback code for when precise stack info is disabled.
Delete fallback code for when copying stacks is disabled.
Delete fallback code for when StackCopyAlways is disabled.
Delete Stktop chain - there is only one stack segment now.
Delete M.moreargp, M.moreargsize, M.moreframesize, M.cret.
Delete G.writenbuf (unrelated, just dead).
Delete runtime.lessstack, runtime.oldstack.
Delete many amd64 morestack variants.
Delete initialization of morestack frame/arg sizes (shortens split prologue!).

Replace G's stackguard/stackbase/stack0/stacksize/
syscallstack/syscallguard/forkstackguard with simple stack
bounds (lo, hi).

Update liblink, runtime/cgo for adjustments to G.

[rsc, 2014-09-09 04:59:16]

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

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

[bradfitz, 2014-09-09 05:17:04]

On Mon, Sep 8, 2014 at 9:59 PM, <rsc@golang.org> wrote:

>
> Affected files (+334, -2876 lines):


Not bad numbers.

Unrelated but related: is reflect.Value going down to 3 words for Go 1.4 or
we waiting to see what happens during Go 1.5 with the GC? People were
asking last week but forgot the plan.

[rsc, 2014-09-09 11:23:12]

On Tue, Sep 9, 2014 at 1:17 AM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:

> On Mon, Sep 8, 2014 at 9:59 PM, <rsc@golang.org> wrote:
>
>>
>> Affected files (+334, -2876 lines):
>
>
> Not bad numbers.
>
> Unrelated but related: is reflect.Value going down to 3 words for Go 1.4
> or we waiting to see what happens during Go 1.5 with the GC? People were
> asking last week but forgot the plan.
>

I'd like to leave reflect alone for 1.4. I think chances are good we won't
need to roll back the 'always a pointer' change, but I am less confident
about that than I am about copying stacks. (This is the second cycle we've
attempted to roll out copying stacks, but only the first for the interface
change.) Assuming we make it through the release with the interface pointer
change intact, we'll be able to apply many simplifications to reflect in
the 1.5 cycle, among them shortening Value to two words.

Russ

[khr, 2014-09-09 15:56:03]

LGTM.

https://codereview.appspot.com/137410043/diff/80001/src/runtime/asm_amd64.s
File src/runtime/asm_amd64.s (left):

https://codereview.appspot.com/137410043/diff/80001/src/runtime/asm_amd64.s#o...
src/runtime/asm_amd64.s:264: // Caller has already done get_tls(CX); MOVQ m(CX),
BX.
Isn't the caller still doing get_tls(CX)?

https://codereview.appspot.com/137410043/diff/80001/src/runtime/asm_amd64p32.s
File src/runtime/asm_amd64p32.s (left):

https://codereview.appspot.com/137410043/diff/80001/src/runtime/asm_amd64p32....
src/runtime/asm_amd64p32.s:237: // Caller has already done get_tls(CX); MOVQ
m(CX), BX.
Same here.

https://codereview.appspot.com/137410043/diff/80001/src/runtime/asm_amd64p32.s
File src/runtime/asm_amd64p32.s (right):

https://codereview.appspot.com/137410043/diff/80001/src/runtime/asm_amd64p32....
src/runtime/asm_amd64p32.s:244: get_tls(CX)
Don't need the get_tls(CX)

https://codereview.appspot.com/137410043/diff/80001/src/runtime/cgo/gcc_darwi...
File src/runtime/cgo/gcc_darwin_386.c (right):

https://codereview.appspot.com/137410043/diff/80001/src/runtime/cgo/gcc_darwi...
src/runtime/cgo/gcc_darwin_386.c:124: ts->g->stackhi = size;
Comment that proc.c fixes this up.

Ditto for all the other gcc_ARCH_OS.c files.

https://codereview.appspot.com/137410043/diff/80001/src/runtime/cgo/gcc_windo...
File src/runtime/cgo/gcc_windows_amd64.c (right):

https://codereview.appspot.com/137410043/diff/80001/src/runtime/cgo/gcc_windo...
src/runtime/cgo/gcc_windows_amd64.c:23: g->stacklo = (uintptr)&tmp - STACKSIZE +
8*1024;
Does this cgo use of stackguard differ from the runtime one?  Because stackguard
and stacklo usually differ by StackGuard, but they don't here.

Maybe the name stackguard was used incorrectly here to begin with, and you're
just fixing it?

https://codereview.appspot.com/137410043/diff/80001/src/runtime/traceback.go
File src/runtime/traceback.go (left):

https://codereview.appspot.com/137410043/diff/80001/src/runtime/traceback.go#...
src/runtime/traceback.go:148: // Hook for handling Windows exception handlers.
See traceback_windows.go.
We don't need this anymore?

[rsc, 2014-09-09 16:13:55]

https://codereview.appspot.com/137410043/diff/80001/src/runtime/asm_amd64.s
File src/runtime/asm_amd64.s (left):

https://codereview.appspot.com/137410043/diff/80001/src/runtime/asm_amd64.s#o...
src/runtime/asm_amd64.s:264: // Caller has already done get_tls(CX); MOVQ m(CX),
BX.
On 2014/09/09 15:56:03, khr wrote:
> Isn't the caller still doing get_tls(CX)?

Yes, but morestack basically never runs anymore, so no harm in doing it again,
and it gives us more isolation between this code and the prologue. (The caller
is definitely not doing MOVQ m(CX), BX anymore.)

https://codereview.appspot.com/137410043/diff/80001/src/runtime/asm_amd64p32.s
File src/runtime/asm_amd64p32.s (left):

https://codereview.appspot.com/137410043/diff/80001/src/runtime/asm_amd64p32....
src/runtime/asm_amd64p32.s:237: // Caller has already done get_tls(CX); MOVQ
m(CX), BX.
On 2014/09/09 15:56:03, khr wrote:
> Same here.

Acknowledged.

https://codereview.appspot.com/137410043/diff/80001/src/runtime/asm_amd64p32.s
File src/runtime/asm_amd64p32.s (right):

https://codereview.appspot.com/137410043/diff/80001/src/runtime/asm_amd64p32....
src/runtime/asm_amd64p32.s:244: get_tls(CX)
On 2014/09/09 15:56:03, khr wrote:
> Don't need the get_tls(CX)

Agreed but I think I'll keep it.

https://codereview.appspot.com/137410043/diff/80001/src/runtime/cgo/gcc_darwi...
File src/runtime/cgo/gcc_darwin_386.c (right):

https://codereview.appspot.com/137410043/diff/80001/src/runtime/cgo/gcc_darwi...
src/runtime/cgo/gcc_darwin_386.c:124: ts->g->stackhi = size;
On 2014/09/09 15:56:03, khr wrote:
> Comment that proc.c fixes this up.
> 
> Ditto for all the other gcc_ARCH_OS.c files.

Done.

https://codereview.appspot.com/137410043/diff/80001/src/runtime/cgo/gcc_windo...
File src/runtime/cgo/gcc_windows_amd64.c (right):

https://codereview.appspot.com/137410043/diff/80001/src/runtime/cgo/gcc_windo...
src/runtime/cgo/gcc_windows_amd64.c:23: g->stacklo = (uintptr)&tmp - STACKSIZE +
8*1024;
On 2014/09/09 15:56:03, khr wrote:
> Does this cgo use of stackguard differ from the runtime one?  Because
stackguard
> and stacklo usually differ by StackGuard, but they don't here.
> 
> Maybe the name stackguard was used incorrectly here to begin with, and you're
> just fixing it?

cgo was trying to convey back to _rt0_go the extent of the actual system stack.
Before it was setting stackguard; now it sets stacklo and lets _rt0_go compute
stackguard.
The net result is that stackguard ends up another StackGuard bytes higher now
than
it did before, but it's noise compared to the stack sizes we are talking about
(here, 2MB - 8kB). Neither is more correct than the other. They're both just
trying to
establish a line near the bottom of the actual stack, but not too near.

https://codereview.appspot.com/137410043/diff/80001/src/runtime/traceback.go
File src/runtime/traceback.go (left):

https://codereview.appspot.com/137410043/diff/80001/src/runtime/traceback.go#...
src/runtime/traceback.go:148: // Hook for handling Windows exception handlers.
See traceback_windows.go.
On 2014/09/09 15:56:03, khr wrote:
> We don't need this anymore?

No, thank goodness.

This hook was here to trace backward past an exception handler frame on a curg
stack,
in particular for the case where the exception handler called other big things
like
printf or throw or whatever and those needed more stack, so we were doing a
traceback in order to decide whether we could copy the stack (and the traceback
would say NO, and we'd make a new segment instead).

The Windows exception handlers now run on the g0 stack, so we don't need to
trace past them on a curg stack, we won't ever see a stack growth during them,
and we don't need to trace past them during panics (if the signal handler itself
decides to crash, it will print a traceback using the context registers; see
os_windows_*.c).

traceback_windows.go has some more scary details.

[rsc, 2014-09-09 17:40:04]

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

runtime: assume precisestack, copystack, StackCopyAlways, ScanStackByFrames

Commit to stack copying for stack growth.

We're carrying around a surprising amount of cruft from older schemes.
I am confident that precise stack scans and stack copying are here to stay.

Delete fallback code for when precise stack info is disabled.
Delete fallback code for when copying stacks is disabled.
Delete fallback code for when StackCopyAlways is disabled.
Delete Stktop chain - there is only one stack segment now.
Delete M.moreargp, M.moreargsize, M.moreframesize, M.cret.
Delete G.writenbuf (unrelated, just dead).
Delete runtime.lessstack, runtime.oldstack.
Delete many amd64 morestack variants.
Delete initialization of morestack frame/arg sizes (shortens split prologue!).

Replace G's stackguard/stackbase/stack0/stacksize/
syscallstack/syscallguard/forkstackguard with simple stack
bounds (lo, hi).

Update liblink, runtime/cgo for adjustments to G.

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

[gobot, 2014-09-09 17:42:14]

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