code review 4188061: ld: detect stack overflow due to NOSPLIT

ld: detect stack overflow due to NOSPLIT

Fix problems found.

On amd64, various library routines had bigger
stack frames than expected, because large function
calls had been added.

runtime.assertI2T: nosplit stack overflow
        120	assumed on entry to runtime.assertI2T
        8	after runtime.assertI2T uses 112
        0	on entry to runtime.newTypeAssertionError
        -8	on entry to runtime.morestack01

runtime.assertE2E: nosplit stack overflow
        120	assumed on entry to runtime.assertE2E
        16	after runtime.assertE2E uses 104
        8	on entry to runtime.panic
        0	on entry to runtime.morestack16
        -8	after runtime.morestack16 uses 8

runtime.assertE2T: nosplit stack overflow
        120	assumed on entry to runtime.assertE2T
        16	after runtime.assertE2T uses 104
        8	on entry to runtime.panic
        0	on entry to runtime.morestack16
        -8	after runtime.morestack16 uses 8

runtime.newselect: nosplit stack overflow
        120	assumed on entry to runtime.newselect
        56	after runtime.newselect uses 64
        48	on entry to runtime.printf
        8	after runtime.printf uses 40
        0	on entry to vprintf
        -8	on entry to runtime.morestack16

runtime.selectdefault: nosplit stack overflow
        120	assumed on entry to runtime.selectdefault
        56	after runtime.selectdefault uses 64
        48	on entry to runtime.printf
        8	after runtime.printf uses 40
        0	on entry to vprintf
        -8	on entry to runtime.morestack16

runtime.selectgo: nosplit stack overflow
        120	assumed on entry to runtime.selectgo
        0	after runtime.selectgo uses 120
        -8	on entry to runtime.gosched

On arm, 5c was tagging functions NOSPLIT that should
not have been, like the recursive function printpanics:

printpanics: nosplit stack overflow
        124	assumed on entry to printpanics
        112	after printpanics uses 12
        108	on entry to printpanics
        96	after printpanics uses 12
        92	on entry to printpanics
        80	after printpanics uses 12
        76	on entry to printpanics
        64	after printpanics uses 12
        60	on entry to printpanics
        48	after printpanics uses 12
        44	on entry to printpanics
        32	after printpanics uses 12
        28	on entry to printpanics
        16	after printpanics uses 12
        12	on entry to printpanics
        0	after printpanics uses 12
        -4	on entry to printpanics

[rsc, 2011-02-18 18:31:05]

Hello r (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-02-18 18:35:10]

Tested on linux/*, darwin/*.
Fails os.TestSeek on arm but otherwise ok.

[r, 2011-02-19 04:26:49]

nice 5c catch.

the 5l thing looks fatal

i am confused about iface.c

the rest looks good. my only question is how expensive you think it is. is it
something we should run every link?

http://codereview.appspot.com/4188061/diff/5001/src/cmd/5l/l.h
File src/cmd/5l/l.h (right):

http://codereview.appspot.com/4188061/diff/5001/src/cmd/5l/l.h#newcode38
src/cmd/5l/l.h:38: thechar = '8',
isn't the char 5?

http://codereview.appspot.com/4188061/diff/5001/src/pkg/runtime/iface.c
File src/pkg/runtime/iface.c (right):

http://codereview.appspot.com/4188061/diff/5001/src/pkg/runtime/iface.c#newco...
src/pkg/runtime/iface.c:327: void
i'm unsure about why all of these lost their pragmas.
convI2I for instance looks harmless - one word of stack.

or to put it another way, some of the functions got broken up so they could
retain their nosplit while others lost it, and i can't figure out the reasoning.

[rsc, 2011-02-22 22:05:52]

> the 5l thing looks fatal

luckily not much code looks at it.
only things in ld/.

> i am confused about iface.c

#pragma textflag 7 is necessary in C when
the C function declaration does not completely
describe the actual parameters + results of a
function.  the typical instance is something
called from Go where the first few params
imply some address calculations to find the
remaining ones.  those have been written with "..."
function arguments just to make that clear.
#pragma textflag 7 is also necessary when a
function inspects or overwrites its return pc,
as in the functions that implement select.

in the cases where the function was doing
too much to merit a textflag 7, i split it so
that the address arithmetic and/or pc manipulation
was kept in the 7 function and the rest deferred
to an ordinary function.

things like ifaceE2I are only dealing with
fixed-size parameter lists (E and I are interface
values, which have fixed size), so they don't
need the #pragma textflag.  it was either a
premature optimization or a copy and paste dreg.
i deleted the textflag in these cases.

> the rest looks good. my only question is how expensive you think it is.
> is it something we should run every link?

i think it is.  it's walking the instruction chains
but they've already been walked a handful of times.
it explores textflag 7 functions once normally
and then once for each call from another textflag 7
function.  so the only repeated expense is for
each different call chain of textflag 7 functions
and there are supposed to be only a small fixed
number of those.

there's no measurable difference running 6l
to generate godoc with and without the call to
dostkcheck.

russ

[r2, 2011-02-22 22:31:21]

LGTM

[rsc, 2011-02-22 22:40:38]

Hello r, r2 (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg

[rsc, 2011-02-22 22:40:46]

*** Submitted as 73af7584f8d9 ***

ld: detect stack overflow due to NOSPLIT

Fix problems found.

On amd64, various library routines had bigger
stack frames than expected, because large function
calls had been added.

runtime.assertI2T: nosplit stack overflow
        120	assumed on entry to runtime.assertI2T
        8	after runtime.assertI2T uses 112
        0	on entry to runtime.newTypeAssertionError
        -8	on entry to runtime.morestack01

runtime.assertE2E: nosplit stack overflow
        120	assumed on entry to runtime.assertE2E
        16	after runtime.assertE2E uses 104
        8	on entry to runtime.panic
        0	on entry to runtime.morestack16
        -8	after runtime.morestack16 uses 8

runtime.assertE2T: nosplit stack overflow
        120	assumed on entry to runtime.assertE2T
        16	after runtime.assertE2T uses 104
        8	on entry to runtime.panic
        0	on entry to runtime.morestack16
        -8	after runtime.morestack16 uses 8

runtime.newselect: nosplit stack overflow
        120	assumed on entry to runtime.newselect
        56	after runtime.newselect uses 64
        48	on entry to runtime.printf
        8	after runtime.printf uses 40
        0	on entry to vprintf
        -8	on entry to runtime.morestack16

runtime.selectdefault: nosplit stack overflow
        120	assumed on entry to runtime.selectdefault
        56	after runtime.selectdefault uses 64
        48	on entry to runtime.printf
        8	after runtime.printf uses 40
        0	on entry to vprintf
        -8	on entry to runtime.morestack16

runtime.selectgo: nosplit stack overflow
        120	assumed on entry to runtime.selectgo
        0	after runtime.selectgo uses 120
        -8	on entry to runtime.gosched

On arm, 5c was tagging functions NOSPLIT that should
not have been, like the recursive function printpanics:

printpanics: nosplit stack overflow
        124	assumed on entry to printpanics
        112	after printpanics uses 12
        108	on entry to printpanics
        96	after printpanics uses 12
        92	on entry to printpanics
        80	after printpanics uses 12
        76	on entry to printpanics
        64	after printpanics uses 12
        60	on entry to printpanics
        48	after printpanics uses 12
        44	on entry to printpanics
        32	after printpanics uses 12
        28	on entry to printpanics
        16	after printpanics uses 12
        12	on entry to printpanics
        0	after printpanics uses 12
        -4	on entry to printpanics

R=r, r2
CC=golang-dev
http://codereview.appspot.com/4188061