code review 140950043: runtime: make entersyscall/exitsyscall safe for stack splits

runtime: make entersyscall/exitsyscall safe for stack splits

It is fundamentally unsafe to grow the stack once someone
has made a call to syscall.Syscall. That function takes 6 uintptr
arguments, but depending on the call some are pointers.
In fact, some might be pointers to stack values, and we don't know which.
That makes it impossible to copy the stack somewhere else.
Since we want to delete all the stack splitting code, relying only
on stack copying, make sure that Syscall never needs to split the stack.

The only thing Syscall does is:
        call entersyscall
        make the system call
        call exitsyscall

As long as we make sure that entersyscall and exitsyscall
can live in the nosplit region, they won't ask for more stack.

Do this by making entersyscall and exitsyscall set up the
stack guard so that any call to a function with a split check
will cause a crash. Then move non-essential slow-path
work onto the m stack using onM and mark the rest of the
work nosplit. The linker will verify that the chain of nosplits
fits in the total nosplit budget.

[rsc, 2014-09-03 20:06:40]

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

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

[iant, 2014-09-03 20:23:37]

LGTM

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

https://codereview.appspot.com/140950043/diff/40001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:436: isscanstatus(uint32 status)
You can now delete isscanstatus.  It's a static function that nobody calls.

https://codereview.appspot.com/140950043/diff/40001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:518: if((oldval&Gscan) || (newval&Gscan) || oldval ==
Gscan || newval == Gscan || oldval == newval) {
If oldval == Gscan, then oldval&Gscan will be non-zero.  You can remove the
check for oldval == Gscan.  Same for newval == Gscan.

https://codereview.appspot.com/140950043/diff/40001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:548: runtime·printf("casgstatus: oldval=%d, newval=%d\n",
oldval, newval);
Note that this is an unimportant change from the previous behaviour. 
Previously, if oldval == Gscan or newval == Gscan, the code would
runtime·throw("isscanstatus: Bad status Gscan"), in isscanstatus.

[rsc, 2014-09-03 21:42:42]

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

runtime: make entersyscall/exitsyscall safe for stack splits

It is fundamentally unsafe to grow the stack once someone
has made a call to syscall.Syscall. That function takes 6 uintptr
arguments, but depending on the call some are pointers.
In fact, some might be pointers to stack values, and we don't know which.
That makes it impossible to copy the stack somewhere else.
Since we want to delete all the stack splitting code, relying only
on stack copying, make sure that Syscall never needs to split the stack.

The only thing Syscall does is:
        call entersyscall
        make the system call
        call exitsyscall

As long as we make sure that entersyscall and exitsyscall
can live in the nosplit region, they won't ask for more stack.

Do this by making entersyscall and exitsyscall set up the
stack guard so that any call to a function with a split check
will cause a crash. Then move non-essential slow-path
work onto the m stack using onM and mark the rest of the
work nosplit. The linker will verify that the chain of nosplits
fits in the total nosplit budget.

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

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

https://codereview.appspot.com/140950043/diff/40001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:436: isscanstatus(uint32 status)
On 2014/09/03 20:23:37, iant wrote:
> You can now delete isscanstatus.  It's a static function that nobody calls.

Not true, there are calls in stopscanstart and mquiesce.

https://codereview.appspot.com/140950043/diff/40001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:518: if((oldval&Gscan) || (newval&Gscan) || oldval ==
Gscan || newval == Gscan || oldval == newval) {
On 2014/09/03 20:23:37, iant wrote:
> If oldval == Gscan, then oldval&Gscan will be non-zero.  You can remove the
> check for oldval == Gscan.  Same for newval == Gscan.

Done.

https://codereview.appspot.com/140950043/diff/40001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:548: runtime·printf("casgstatus: oldval=%d, newval=%d\n",
oldval, newval);
On 2014/09/03 20:23:37, iant wrote:
> Note that this is an unimportant change from the previous behaviour. 
> Previously, if oldval == Gscan or newval == Gscan, the code would
> runtime·throw("isscanstatus: Bad status Gscan"), in isscanstatus.

Acknowledged.

[iant, 2014-09-03 21:44:21]

On Wed, Sep 3, 2014 at 2:42 PM,  <rsc@golang.org> wrote:
> On 2014/09/03 20:23:37, iant wrote:
>>
>> You can now delete isscanstatus.  It's a static function that nobody
>
> calls.
>
> Not true, there are calls in stopscanstart and mquiesce.

Whoops, sorry, reitveld defeated me.

Ian

[gobot, 2014-09-03 21:45:18]

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

[gobot, 2014-09-04 01:50:29]

R=

[gobot, 2014-09-04 01:51:04]

I am a dumb robot.

—gobot

[rsc, 2014-09-04 01:58:04]

Sorry for the spam. Testing the CL poster for adg.

On Wed, Sep 3, 2014 at 9:51 PM, <gobot@golang.org> wrote:

> I am a dumb robot.
>
> --gobot
>

While I'm here, WTF? I sent Rietveld —gobot and it turned into --gobot.
Sigh.

Russ

[dave_cheney.net, 2014-09-04 01:59:24]

Gotta be careful with those uft-8 characters where python is concerned.

On Thu, Sep 4, 2014 at 11:58 AM, Russ Cox <rsc@golang.org> wrote:
> Sorry for the spam. Testing the CL poster for adg.
>
> On Wed, Sep 3, 2014 at 9:51 PM, <gobot@golang.org> wrote:
>>
>> I am a dumb robot.
>>
>> --gobot
>
>
> While I'm here, WTF? I sent Rietveld —gobot and it turned into --gobot.
> Sigh.
>
> Russ
>
> --
> 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/d/optout.