code review 13421048: runtime: avoid inconsistent goroutine state in profiler

runtime: avoid inconsistent goroutine state in profiler

Because profiling signals can arrive at any time, we must
handle the case where a profiling signal arrives halfway
through a goroutine switch. Luckily, although there is much
to think through, very little needs to change.

Fixes issue 6000.
Fixes issue 6015.

[rsc, 2013-09-13 04:25:37]

Hello golang-dev@googlegroups.com,

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

[dvyukov, 2013-09-13 04:44:06]

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

https://codereview.appspot.com/13421048/diff/8001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:2118: // consistent.
uh
I am curious can't it be done in some other simpler, less arch-dependent way?..
does the test tests all the cases? I do not see signals there.

[dvyukov, 2013-09-13 04:50:33]

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

https://codereview.appspot.com/13421048/diff/8001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:2061: // signal handler. The operating system updates PC
and SP first, and then
we do not use nested signals (sa.sa_mask = ~0ULL), which is reasonable, because
we do not execute user code in handlers
so the part regarding signals must be irrelevant, they are completely invisible
to SIGPROF

[dvyukov, 2013-09-13 04:51:50]

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

https://codereview.appspot.com/13421048/diff/8001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:2074: // When switching from a system g to a user g,
there are four possibilities.
is it 3?
or one is missed in the description below?

[dvyukov, 2013-09-13 05:03:18]

https://codereview.appspot.com/13421048/diff/8001/src/pkg/runtime/asm_amd64.s
File src/pkg/runtime/asm_amd64.s (right):

https://codereview.appspot.com/13421048/diff/8001/src/pkg/runtime/asm_amd64.s...
src/pkg/runtime/asm_amd64.s:151: MOVQ	SI, g(CX)
will it be simpler (and possible) to put gobuf_pc onto stack, and then switch to
user g with RET?
in the window between MOVQ SI, g(CX) and RET, it will look like user goroutine
has called gogo
but it can show only in profiles

[rsc, 2013-09-13 13:52:44]

https://codereview.appspot.com/13421048/diff/8001/src/pkg/runtime/asm_amd64.s
File src/pkg/runtime/asm_amd64.s (right):

https://codereview.appspot.com/13421048/diff/8001/src/pkg/runtime/asm_amd64.s...
src/pkg/runtime/asm_amd64.s:151: MOVQ	SI, g(CX)
On 2013/09/13 05:03:18, dvyukov wrote:
> will it be simpler (and possible) to put gobuf_pc onto stack, and then switch
to
> user g with RET?
> in the window between MOVQ SI, g(CX) and RET, it will look like user goroutine
> has called gogo
> but it can show only in profiles

That's a very big change and not one I am comfortable with for Go 1.2.
It is a fundamental change to the way gogo works, and I am not even sure it is
okay.
It also only applies on x86, because gogo needs to restore both PC and LR on
ARM.
We'd still need to recognize the PC update on ARM. I'd rather keep everything 
more or less the same and recognize the PC update on all the architectures.

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

https://codereview.appspot.com/13421048/diff/8001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:2061: // signal handler. The operating system updates PC
and SP first, and then
On 2013/09/13 04:50:33, dvyukov wrote:
> we do not use nested signals (sa.sa_mask = ~0ULL), which is reasonable,
because
> we do not execute user code in handlers
> so the part regarding signals must be irrelevant, they are completely
invisible
> to SIGPROF

OK, I will add a note about this not being possible today.

https://codereview.appspot.com/13421048/diff/8001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:2074: // When switching from a system g to a user g,
there are four possibilities.
On 2013/09/13 04:51:50, dvyukov wrote:
> is it 3?
> or one is missed in the description below?

It's three. I merged the asmcgocall and cgocallback_gofunc cases into one. Will
fix.

https://codereview.appspot.com/13421048/diff/8001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:2118: // consistent.
On 2013/09/13 04:44:06, dvyukov wrote:
> uh
> I am curious can't it be done in some other simpler, less arch-dependent
way?..
> does the test tests all the cases? I do not see signals there.

I don't believe it can. This is a long comment but a tiny amount of actual
change. I don't want to make more significant changes before Go 1.2 anyway.

The test tests many of the cases, but not all of them. Testing all of them is
difficult. I tested more by hand by changing the gogo assembly to be wrong in
various ways.

[dvyukov, 2013-09-13 16:00:08]

https://codereview.appspot.com/13421048/diff/19001/src/pkg/runtime/crash_cgo_...
File src/pkg/runtime/crash_cgo_test.go (right):

https://codereview.appspot.com/13421048/diff/19001/src/pkg/runtime/crash_cgo_...
src/pkg/runtime/crash_cgo_test.go:15: t.Skip("FAIL")
is it related?

[rsc, 2013-09-13 16:30:02]

Sorry, you've caught my CL in an inconsistent state. :-)

I uploaded it because I was moving it between my arm and x86 machines. I
will send a PTAL when it's ready to be reviewed again. I'm doing something
a little different, even simpler.

Russ

[rsc, 2013-09-13 17:22:18]

PTAL

Even simpler.
Ignoring tests and comments, the CL is now 5 lines of C code (3 in headers,
2 in proc.c).
No assembly changes.
Tested on amd64, 386, arm.

[dvyukov, 2013-09-13 18:11:31]

LGTM
yes, this is much simpler, thanks

https://codereview.appspot.com/13421048/diff/27001/src/pkg/runtime/runtime_te...
File src/pkg/runtime/runtime_test.go (right):

https://codereview.appspot.com/13421048/diff/27001/src/pkg/runtime/runtime_te...
src/pkg/runtime/runtime_test.go:107: out, err = exec.Command("go", "tool", "nm",
"-S", dir+"/hello").CombinedOutput()
won't windows build hello.exe?

[rsc, 2013-09-13 18:19:05]

https://codereview.appspot.com/13421048/diff/27001/src/pkg/runtime/runtime_te...
File src/pkg/runtime/runtime_test.go (right):

https://codereview.appspot.com/13421048/diff/27001/src/pkg/runtime/runtime_te...
src/pkg/runtime/runtime_test.go:107: out, err = exec.Command("go", "tool", "nm",
"-S", dir+"/hello").CombinedOutput()
On 2013/09/13 18:11:31, dvyukov wrote:
> won't windows build hello.exe?

no, not if i say -o hello.

[rsc, 2013-09-13 18:19:26]

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

runtime: avoid inconsistent goroutine state in profiler

Because profiling signals can arrive at any time, we must
handle the case where a profiling signal arrives halfway
through a goroutine switch. Luckily, although there is much
to think through, very little needs to change.

Fixes issue 6000.
Fixes issue 6015.

R=golang-dev, dvyukov
CC=golang-dev
https://codereview.appspot.com/13421048

[brainman, 2013-09-14 05:42:01]

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

https://codereview.appspot.com/13421048/diff/33001/src/pkg/runtime/proc.c#new...
src/pkg/runtime/proc.c:2119: ((uint8*)runtime·gogo <= pc && pc <
(uint8*)runtime·gogo + RuntimeGogoBytes))
This new condition does not work on windows (broken runtime/pprof tests). I
suspect some of these assumptions don't hold on windows. It has been a while
:-), I don't remember how it is. I will try to fix it when I have time.