code review 7304104: runtime: allow cgo callbacks on non-Go threads

runtime: allow cgo callbacks on non-Go threads

Fixes issue 4435.

[rsc, 2013-02-18 17:42:56]

Hello golang-dev@googlegroups.com,

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

[iant, 2013-02-18 20:10:50]

https://codereview.appspot.com/7304104/diff/2001/misc/cgo/test/cthread.c
File misc/cgo/test/cthread.c (right):

https://codereview.appspot.com/7304104/diff/2001/misc/cgo/test/cthread.c#newc...
misc/cgo/test/cthread.c:28: pthread_create(&thread_id[i], &attr, addThread,
&max);
Why bother with attr and pthread_attr_init?  Since you aren't changing any
values, you may as well pass NULL to pthread_create.

https://codereview.appspot.com/7304104/diff/2001/misc/cgo/test/cthread.c#newc...
misc/cgo/test/cthread.c:28: pthread_create(&thread_id[i], &attr, addThread,
&max);
check that nthread <= nelem(thread_id) ?

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/asm_386.s
File src/pkg/runtime/asm_386.s (right):

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/asm_386.s#ne...
src/pkg/runtime/asm_386.s:498: MOVL	m(CX), BP
You can move the MOVL m(CX),BP line before the havem label.

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

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/asm_amd64.s#...
src/pkg/runtime/asm_amd64.s:531: MOVQ	m(CX), BP
You can move the MOVQ m(CX),BP before the havem label.

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/asm_amd64.s#...
src/pkg/runtime/asm_amd64.s:597: MOVQ	$runtime·dropm(SB), AX
What will happen if C code calls Go code with no m, and the Go code calls panic?

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/cgocall.c
File src/pkg/runtime/cgocall.c (right):

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/cgocall.c#ne...
src/pkg/runtime/cgocall.c:220: m->needextram = 1;
Seems like needextram is already 1, not sure why you are setting it to 1.

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

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:998: // to release the m on thread exit. With this
approach we cannot in
Seems like you could use pthread_key_create to allocate a slot for the m, and
also use the TLS variable m that we are already using.  That would give you the
convenient destructor without requiring you to change any of the existing code. 
Ordinary Go threads would not need to call pthread_setspecific for this key, as
they would never exit; only threads we first see for a callback would need to.

[brainman, 2013-02-18 22:33:14]

Your test fails on windows.

# ..\misc\cgo\test
# _/C_/go/root/misc/cgo/test
..\misc\cgo\test\cthread.c:5:21: fatal error: pthread.h: No such file or
directory
compilation terminated.
FAIL    _/C_/go/root/misc/cgo/test [build failed]

Alex

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/thread_windo...
File src/pkg/runtime/thread_windows.c (right):

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/thread_windo...
src/pkg/runtime/thread_windows.c:217: {
USED(p);USED(n);

[brainman, 2013-02-18 22:33:59]

On 2013/02/18 22:33:14, brainman wrote:
> USED(p);USED(n);

Should do the same to thread_plan9.c.

Alex

[minux1, 2013-02-19 20:40:08]

this breaks ARM builds, I don't yet know why
perhaps I made a wrong assumption somewhere,
feel free to submit when it's ready.
i will hunt the ARM bug.

# ../misc/cgo/test
panic: runtime error: call of nil func value [recovered]
	panic: runtime error: call of nil func value
[signal 0xb code=0x1 addr=0x0 pc=0x0]

goroutine 13 [running]:
testing.func·004(0x40051fd4, 0x1026a100)
	/home/minux/go.hg/src/pkg/testing/testing.go:310 +0x148
created by testing.RunTests
	/home/minux/go.hg/src/pkg/testing/testing.go:430 +0x6d4

goroutine 1 [chan receive]:
testing.RunTests(0x10c00, 0x19e578, 0x1b, 0x1b, 0x1, ...)
	/home/minux/go.hg/src/pkg/testing/testing.go:431 +0x6f4
testing.Main(0x10c00, 0x19e578, 0x1b, 0x1b, 0x19bb70, ...)
	/home/minux/go.hg/src/pkg/testing/testing.go:327 +0x68
main.main()
	_/home/minux/go.hg/misc/cgo/test/_test/_testmain.go:97 +0x94
exit status 2
FAIL	_/home/minux/go.hg/misc/cgo/test	0.033s
Command exited with non-zero status 1

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/asm_arm.s
File src/pkg/runtime/asm_arm.s (right):

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/asm_arm.s#ne...
src/pkg/runtime/asm_arm.s:376: CALL	(R0)
s/CALL/BL/

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

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:904: // is - once it has installed its own m so that ican
do things like
s/ican/it can/

[rsc, 2013-02-19 20:59:38]

PTAL

Thanks for all the feedback. 

Alex, I split cthread.c into cthread_unix.c and cthread_windows.c. I attempted
to write cthread_windows.c but I haven't tested on Windows (that laptop had to
be reformatted a while back and I haven't recreated it).

Minux, I have a Chromebook I can test the arm code on, but not with me today.
I'll try to do that later this week, but if you have time to poke at why arm
doesn't work, that'd be great.

Russ

https://codereview.appspot.com/7304104/diff/2001/misc/cgo/test/cthread.c
File misc/cgo/test/cthread.c (right):

https://codereview.appspot.com/7304104/diff/2001/misc/cgo/test/cthread.c#newc...
misc/cgo/test/cthread.c:28: pthread_create(&thread_id[i], &attr, addThread,
&max);
On 2013/02/18 20:10:50, iant wrote:
> Why bother with attr and pthread_attr_init?  Since you aren't changing any
> values, you may as well pass NULL to pthread_create.

I just copy and paste code when I need to create a pthread. Done.

https://codereview.appspot.com/7304104/diff/2001/misc/cgo/test/cthread.c#newc...
misc/cgo/test/cthread.c:28: pthread_create(&thread_id[i], &attr, addThread,
&max);
On 2013/02/18 20:10:50, iant wrote:
> check that nthread <= nelem(thread_id) ?

Done.

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/asm_386.s
File src/pkg/runtime/asm_386.s (right):

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/asm_386.s#ne...
src/pkg/runtime/asm_386.s:498: MOVL	m(CX), BP
On 2013/02/18 20:10:50, iant wrote:
> You can move the MOVL m(CX),BP line before the havem label.

Done.

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

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/asm_amd64.s#...
src/pkg/runtime/asm_amd64.s:531: MOVQ	m(CX), BP
On 2013/02/18 20:10:50, iant wrote:
> You can move the MOVQ m(CX),BP before the havem label.

Done.

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/asm_amd64.s#...
src/pkg/runtime/asm_amd64.s:597: MOVQ	$runtime·dropm(SB), AX
On 2013/02/18 20:10:50, iant wrote:
> What will happen if C code calls Go code with no m, and the Go code calls
panic?

I think it all works, because while the Go code is executing there is an m. The
test has an implicit (nil deref) panic and recover in the called Go code. Is
there a specific reason you are worried?

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/asm_arm.s
File src/pkg/runtime/asm_arm.s (right):

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/asm_arm.s#ne...
src/pkg/runtime/asm_arm.s:376: CALL	(R0)
On 2013/02/19 20:40:08, minux wrote:
> s/CALL/BL/

Done.

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/cgocall.c
File src/pkg/runtime/cgocall.c (right):

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/cgocall.c#ne...
src/pkg/runtime/cgocall.c:220: m->needextram = 1;
On 2013/02/18 20:10:50, iant wrote:
> Seems like needextram is already 1, not sure why you are setting it to 1.

Oops, that's supposed to be a 0. :-)

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

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:904: // is - once it has installed its own m so that ican
do things like
On 2013/02/19 20:40:08, minux wrote:
> s/ican/it can/

Done.

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:998: // to release the m on thread exit. With this
approach we cannot in
On 2013/02/18 20:10:50, iant wrote:
> Seems like you could use pthread_key_create to allocate a slot for the m, and
> also use the TLS variable m that we are already using.  That would give you
the
> convenient destructor without requiring you to change any of the existing
code. 
> Ordinary Go threads would not need to call pthread_setspecific for this key,
as
> they would never exit; only threads we first see for a callback would need to.

Thanks. I really like this suggestion. Not much here changes, we just call dropm
from a different place. I'd like to push forward with the current version and do
the pthread destructor in a separate CL, as a performance optimization.

I've updated the comment.

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/thread_windo...
File src/pkg/runtime/thread_windows.c (right):

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/thread_windo...
src/pkg/runtime/thread_windows.c:217: {
On 2013/02/18 22:33:15, brainman wrote:
> USED(p);USED(n);

Done.

[brainman, 2013-02-20 00:21:42]

windows/386 problems:

This is the end of the build:

...

# ..\misc\cgo\stdio

# ..\misc\cgo\test
# _/C_/go/root/misc/cgo/test
..\misc\cgo\test\cthread_windows.c: In function 'addThread':
..\misc\cgo\test\cthread_windows.c:18:2: warning: 'return' with a value, in
function returning void
..\misc\cgo\test\cthread_windows.c: In function 'doAdd':
..\misc\cgo\test\cthread_windows.c:31:3: warning: passing argument 3 of
'_beginthreadex' from incompatible pointer type
c:\go\mingw\bin\../lib/gcc/mingw32/4.5.2/../../../../include/process.h:100:2:
note: expected 'unsigned int (*)(void *)' but argument is of type 'void (*)(void
*)'
ok      _/C_/go/root/misc/cgo/test      19.469s

# ..\doc\progs

# ..\test

# Checking API compatibility.
+pkg go/types, method (*Const) GetPkg() *Package
+pkg go/types, method (*Func) GetPkg() *Package
+pkg go/types, method (*Package) GetPkg() *Package
+pkg go/types, method (*TypeName) GetPkg() *Package
+pkg go/types, method (*Var) GetPkg() *Package
+pkg go/types, type Const struct, Pkg *Package
+pkg go/types, type Func struct, Pkg *Package
+pkg go/types, type Object interface, GetPkg() *Package
+pkg go/types, type TypeName struct, Pkg *Package
+pkg go/types, type Var struct, Pkg *Package
~pkg net, func ListenUnixgram(string, *UnixAddr) (*UDPConn, error)
~pkg text/template/parse, type DotNode bool
~pkg text/template/parse, type Node interface { Copy, String, Type }
±pkg go/types, type Importer func(imports map[string]*Package, path string) (pkg
 *Package, err error)

ALL TESTS PASSED

...

So, there are some compiler warnings. But misc\cgo\test test executable also
crashes. You don't get to see the crash, because, my "system debugger" is
started to handle the exception, and I had to take some manual options to kill
the bugger. I think it is because your new C threads don't set any exception
handler. Our builder won't be able to do what I did, so we have to be careful.

I tried to poke at it:

C:\go\root\misc\cgo\test>gdb test.test.exe
GNU gdb (GDB) 7.2
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "mingw32".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from C:\go\root\misc\cgo\test/test.test.exe...done.
(gdb) r
Starting program: C:\go\root\misc\cgo\test/test.test.exe
[New Thread 6492.0xe08]
[New Thread 6492.0x1da8]
[New Thread 6492.0x1eec]
scatter = 0042D268
hello from C
[New Thread 6492.0x178c]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 6492.0x178c]
runtime.cgocallback (fn=void, frame=void, framesize=void)
    at C:/go/root/src/pkg/runtime/asm_386.s:485
485             MOVL    m(CX), BP
(gdb) bt
#0  runtime.cgocallback (fn=void, frame=void, framesize=void)
    at C:/go/root/src/pkg/runtime/asm_386.s:485
#1  0x0042c500 in _cgoexp_e9125f37873d_Add (a=void, n=void)
    at
C:/DOCUME~1/brainman/LOCALS~1/Temp/go-build214494323/_/C_/go/root/misc/cgo/test/_test/_cgo_defun.c:321
#2  0x004805f6 in ?? ()
#3  0x0042ca5b in ?? ()
#4  0x0042d3dc in ?? ()
#5  0x77c3a3b0 in msvcrt!_endthreadex () from C:\WINDOWS\system32\msvcrt.dll
#6  0x7c80b729 in KERNEL32!GetModuleFileNameA ()
   from C:\WINDOWS\system32\kernel32.dll
#7  0x00000000 in ?? ()
(gdb) info reg
eax            0x4      4
ecx            0x0      0
edx            0xad0608 11339272
ebx            0x0      0
esp            0x3152fefc       0x3152fefc
ebp            0x3152ff30       0x3152ff30
esi            0xa      10
edi            0x0      0
eip            0x41c4ca 0x41c4ca <runtime.cgocallback+10>
eflags         0x10216  [ PF AF IF RF ]
cs             0x1b     27
ss             0x23     35
ds             0x23     35
es             0x23     35
fs             0x3b     59
gs             0x0      0

But, I don't know what I am looking for.

The windows/amd64 didn't get very far either:

pkg/go/doc
pkg/go/build
cmd/go
lockextra: nosplit stack overflow
        120     assumed on entry to lockextra
        88      after lockextra uses 32
        80      on entry to runtime.osyield
        56      after runtime.osyield uses 24
        48      on entry to runtime.stdcall
        -16     after runtime.stdcall uses 64
go tool dist: FAILED: c:\MinGW64\go\pkg\tool\windows_amd64\6l -o
c:\MinGW64\go\pkg\tool\windows_amd64\go_bootstrap.exe
C:\Users\brainman\AppData\Local\Temp\goEC42.tmp\_go_.6

Alex

[iant, 2013-02-20 02:01:28]

On Tue, Feb 19, 2013 at 12:59 PM,  <rsc@golang.org> wrote:
>>
>> What will happen if C code calls Go code with no m, and the Go code
>
> calls panic?
>
> I think it all works, because while the Go code is executing there is an
> m. The test has an implicit (nil deref) panic and recover in the called
> Go code. Is there a specific reason you are worried?

I was worried that perhaps nothing would call dropm in that case.

If you switch to the pthread_key_create destructor this is clearly a
non-issue (and it may be a non-issue anyhow, I'm not sure).

Ian

[iant, 2013-02-20 02:56:54]

LGTM

[dvyukov, 2013-02-20 07:45:35]

https://codereview.appspot.com/7304104/diff/6002/misc/cgo/test/cthread.go
File misc/cgo/test/cthread.go (right):

https://codereview.appspot.com/7304104/diff/6002/misc/cgo/test/cthread.go#new...
misc/cgo/test/cthread.go:21: func Add(x int) {
also call runtime.Gosched() just in case

https://codereview.appspot.com/7304104/diff/6002/misc/cgo/test/cthread.go#new...
misc/cgo/test/cthread.go:32: func testCthread(t *testing.T) {
run it with GOMAXPROCS>1 as well
when GOMAXPROCS=1 threads do not actually collide in Add(), so no goroutines
block on the sum mutex

https://codereview.appspot.com/7304104/diff/6002/misc/cgo/test/cthread.go#new...
misc/cgo/test/cthread.go:33: C.doAdd(10, 6)
it makes sense to use more iterations, so that the threads actually collide at
the callback

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

https://codereview.appspot.com/7304104/diff/6002/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:807: runtime·newextram();
if(runtime·iscgo)?

https://codereview.appspot.com/7304104/diff/6002/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:940: runtime·setmg(mp, mp->g0);
Why is it running on g0?
runtime.gosched() will throw if called on g0.

https://codereview.appspot.com/7304104/diff/6002/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:945: runtime·minit();
What about asminit()?
asm_386 switch FPU to a different mode. This means that Go code will behave
differently depending on how it is called.

https://codereview.appspot.com/7304104/diff/6002/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1004: // but without pthreads, like Windows.
FYI: Windows also has thread completion notifications if you do something along
the lines of:

void __stdcall on_tls_callback(void* hinst, unsigned long reason, void* unused)
{
    (void)hinst;
    (void)unused;
    if (reason == 3)
        tss_dtor();
}

#pragma data_seg(push, old_seg)
#ifdef RL_M32
#   pragma data_seg(".CRT$XLB")
#else
#   pragma const_seg(".CRT$XLB")
#endif
__declspec(dllexport) void (__stdcall * volatile p_on_tls_callback)(void*,
unsigned long, void*) = on_tls_callback;
#pragma data_seg(pop, old_seg)

https://codereview.appspot.com/7304104/diff/6002/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1011: runtime·setprof(false);
It's already disabled by entersyscall.

[rsc, 2013-02-20 15:17:48]

On Tue, Feb 19, 2013 at 9:01 PM, Ian Lance Taylor <iant@golang.org> wrote:

> On Tue, Feb 19, 2013 at 12:59 PM,  <rsc@golang.org> wrote:
> >>
> >> What will happen if C code calls Go code with no m, and the Go code
> >
> > calls panic?
> >
> > I think it all works, because while the Go code is executing there is an
> > m. The test has an implicit (nil deref) panic and recover in the called
> > Go code. Is there a specific reason you are worried?
>
> I was worried that perhaps nothing would call dropm in that case.
>
> If you switch to the pthread_key_create destructor this is clearly a
> non-issue (and it may be a non-issue anyhow, I'm not sure).
>

I think panic is fine. Since we're entering on a brand new m, there are no
deferred recovers on entry. If a recover deferred within the callback stops
the panic, as in the test, then things work fine, because the callback
returns normally. If not, then the whole process will exit after printing
the usual goroutine dump, so that should be fine too.

If the callback into Go calls into C and then C calls pthread_exit, then we
will indeed leak the m, but that's true of ordinary Go threads too. (And
worse things probably happen too, since Go thinks the m and the thread
still exist. I'm not too worried about that case, at least today.)

Russ

[rsc, 2013-02-20 15:25:29]

On Tue, Feb 19, 2013 at 7:21 PM, <alex.brainman@gmail.com> wrote:

> Program received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 6492.0x178c]
> runtime.cgocallback (fn=void, frame=void, framesize=void)
>     at C:/go/root/src/pkg/runtime/**asm_386.s:485
> 485             MOVL    m(CX), BP
> (gdb) bt
>

Thanks for this information. I believe the problem is that 0x14(FS) (now
CX) is nil here, so in addition to setting m and g we need to set the
14(FS) space. On Darwin and OS X there is not this extra indirect. The code
here needs to test whether CX is zero and handle that, but then setmg needs
to initialize it too. And probably it has to be preserved the same way that
externalthreadhandler does today.

I will set up a Windows laptop to try this out.

Russ

[rsc, 2013-02-20 15:28:20]

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

https://codereview.appspot.com/7304104/diff/6002/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:807: runtime·newextram();
On 2013/02/20 07:45:35, dvyukov wrote:
> if(runtime·iscgo)?

Done.

https://codereview.appspot.com/7304104/diff/6002/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:940: runtime·setmg(mp, mp->g0);
On 2013/02/20 07:45:35, dvyukov wrote:
> Why is it running on g0?
> runtime.gosched() will throw if called on g0.

At this point we're in the system thread, so we're running on the system thread
goroutine, which is m->g0. When needm returns, the code will jump into
cgocallbackg, which will switch us off m->g0 onto m->curg in order to run the
actual callback.

https://codereview.appspot.com/7304104/diff/6002/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:945: runtime·minit();
On 2013/02/20 07:45:35, dvyukov wrote:
> What about asminit()?
> asm_386 switch FPU to a different mode. This means that Go code will behave
> differently depending on how it is called.

Done.

https://codereview.appspot.com/7304104/diff/6002/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1004: // but without pthreads, like Windows.
Thanks. I would never have come up with that. :-)

https://codereview.appspot.com/7304104/diff/6002/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1011: runtime·setprof(false);
On 2013/02/20 07:45:35, dvyukov wrote:
> It's already disabled by entersyscall.

Done.

[dvyukov, 2013-02-20 15:31:49]

On 2013/02/20 15:28:20, rsc wrote:

>
https://codereview.appspot.com/7304104/diff/6002/src/pkg/runtime/proc.c#newco...
> src/pkg/runtime/proc.c:940: runtime·setmg(mp, mp->g0);
> On 2013/02/20 07:45:35, dvyukov wrote:
> > Why is it running on g0?
> > runtime.gosched() will throw if called on g0.
> 
> At this point we're in the system thread, so we're running on the system
thread
> goroutine, which is m->g0. When needm returns, the code will jump into
> cgocallbackg, which will switch us off m->g0 onto m->curg in order to run the
> actual callback.

Ah, I see.

>
https://codereview.appspot.com/7304104/diff/6002/src/pkg/runtime/proc.c#newco...
> src/pkg/runtime/proc.c:945: runtime·minit();
> On 2013/02/20 07:45:35, dvyukov wrote:
> > What about asminit()?
> > asm_386 switch FPU to a different mode. This means that Go code will behave
> > differently depending on how it is called.
> 
> Done.

However, then we change behavior of the C code on that thread...

[rsc, 2013-02-20 15:36:37]

On Wed, Feb 20, 2013 at 10:31 AM, <dvyukov@google.com> wrote:

> However, then we change behavior of the C code on that thread...
>

If it had better manners, the C thread would thank us.

Russ

[rsc, 2013-02-20 22:02:11]

The current version of the CL has been revised and tested to work on:

darwin/386
darwin/amd64
linux/386
linux/amd64
linux/arm*
windows/386
 windows/amd64

The test is flaky (fails <10% of the time) on arm. I don't know what's
wrong. It could be general memory corruption unrelated to this CL.

Russ

[rsc, 2013-02-20 22:41:57]

I played some more with ARM and got nowhere. Disabling the test on ARM and
submitting. Maybe someone else can poke at the ARM problems.

Russ

[brainman, 2013-02-20 22:43:30]

It is still crashing in misc/cgo/test on windows/386. But I have nothing to
report yet.

Alex

[rsc, 2013-02-20 22:48:27]

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

runtime: allow cgo callbacks on non-Go threads

Fixes issue 4435.

R=golang-dev, iant, alex.brainman, minux.ma, dvyukov
CC=golang-dev
https://codereview.appspot.com/7304104

[rsc, 2013-02-20 22:50:03]

On Wed, Feb 20, 2013 at 5:43 PM, <alex.brainman@gmail.com> wrote:

> It is still crashing in misc/cgo/test on windows/386. But I have nothing
> to report yet.
>

Let me know if what I just submitted is crashing for you. I had the test
passing while testing on a Windows machine, but I might have messed up
copying the changes back to my local machine.

Russ

[brainman, 2013-02-20 23:49:50]

misc/cgo/test crashes sometimes. Perhaps, this will help you:

(gdb) disas
Dump of assembler code for function runtime.usleep:
   0x00416680 <+0>:     mov    %fs:0x14,%ecx
=> 0x00416687 <+7>:     mov    (%ecx),%ecx
   0x00416689 <+9>:     cmp    (%ecx),%esp
   0x0041668b <+11>:    ja     0x416699 <runtime.usleep+25>
   0x0041668d <+13>:    xor    %edx,%edx
   0x0041668f <+15>:    mov    $0x4,%eax
   0x00416694 <+20>:    call   0x41c470 <runtime.morestack>
   0x00416699 <+25>:    sub    $0xc,%esp
   0x0041669c <+28>:    mov    0x10(%esp),%ecx
   0x004166a0 <+32>:    mov    $0x10624dd3,%eax
   0x004166a5 <+37>:    mul    %ecx
   0x004166a7 <+39>:    shr    $0x6,%edx
   0x004166aa <+42>:    mov    %edx,%ecx
   0x004166ac <+44>:    cmp    $0x0,%edx
   0x004166af <+47>:    jne    0x4166b6 <runtime.usleep+54>
   0x004166b1 <+49>:    mov    $0x1,%ecx
   0x004166b6 <+54>:    mov    0x58a0c4,%eax
   0x004166bc <+60>:    mov    %eax,(%esp)
   0x004166bf <+63>:    mov    $0x1,%eax
   0x004166c4 <+68>:    mov    %eax,0x4(%esp)
   0x004166c8 <+72>:    mov    %ecx,0x8(%esp)
   0x004166cc <+76>:    call   0x416ad0 <runtime.stdcall>
   0x004166d1 <+81>:    add    $0xc,%esp
   0x004166d4 <+84>:    ret
End of assembler dump.
(gdb) info r
eax            0x1      1
ecx            0x0      0
edx            0x0      0
ebx            0x0      0
esp            0x3182fecc       0x3182fecc
ebp            0x3182ff30       0x3182ff30
esi            0xa      10
edi            0x0      0
eip            0x416687 0x416687 <runtime.usleep+7>
eflags         0x10246  [ PF ZF IF RF ]
cs             0x1b     27
ss             0x23     35
ds             0x23     35
es             0x23     35
fs             0x3b     59
gs             0x0      0
(gdb) bt
#0  0x00416687 in runtime.usleep (us=void)
    at C:/go/root/src/pkg/runtime/thread_windows.c:146
#1  0x004121bd in lockextra (nilokay=void)
    at C:/go/root/src/pkg/runtime/proc.c:1044
#2  0x00411fbc in runtime.needm (x=void)
    at C:/go/root/src/pkg/runtime/proc.c:915
#3  0x0041c70c in runtime.cgocallback (fn=void, frame=void, framesize=void)
    at C:/go/root/src/pkg/runtime/asm_386.s:495
#4  0x0042c780 in _cgoexp_e9125f37873d_Add (a=void, n=void)
    at
C:/DOCUME~1/brainman/LOCALS~1/Temp/go-build908326399/_/C_/go/root/misc/cgo/test/_test/_cgo_defun.c:321
#5  0x00480836 in ?? ()
#6  0x0042ccdb in ?? ()
#7  0x0042d65c in ?? ()
#8  0x77c3a3b0 in msvcrt!_endthreadex () from C:\WINDOWS\system32\msvcrt.dll
#9  0x7c80b729 in KERNEL32!GetModuleFileNameA ()
   from C:\WINDOWS\system32\kernel32.dll
#10 0x00000000 in ?? ()
(gdb)

[dvyukov, 2013-02-21 05:04:32]

I guess we need to mark usleep/osyield as #pragma textflag,7 (no split stack).


On Thu, Feb 21, 2013 at 3:49 AM,  <alex.brainman@gmail.com> wrote:
> misc/cgo/test crashes sometimes. Perhaps, this will help you:
>
> (gdb) disas
> Dump of assembler code for function runtime.usleep:
>    0x00416680 <+0>:     mov    %fs:0x14,%ecx
> => 0x00416687 <+7>:     mov    (%ecx),%ecx
>    0x00416689 <+9>:     cmp    (%ecx),%esp
>    0x0041668b <+11>:    ja     0x416699 <runtime.usleep+25>
>    0x0041668d <+13>:    xor    %edx,%edx
>    0x0041668f <+15>:    mov    $0x4,%eax
>    0x00416694 <+20>:    call   0x41c470 <runtime.morestack>
>    0x00416699 <+25>:    sub    $0xc,%esp
>    0x0041669c <+28>:    mov    0x10(%esp),%ecx
>    0x004166a0 <+32>:    mov    $0x10624dd3,%eax
>    0x004166a5 <+37>:    mul    %ecx
>    0x004166a7 <+39>:    shr    $0x6,%edx
>    0x004166aa <+42>:    mov    %edx,%ecx
>    0x004166ac <+44>:    cmp    $0x0,%edx
>    0x004166af <+47>:    jne    0x4166b6 <runtime.usleep+54>
>    0x004166b1 <+49>:    mov    $0x1,%ecx
>    0x004166b6 <+54>:    mov    0x58a0c4,%eax
>    0x004166bc <+60>:    mov    %eax,(%esp)
>    0x004166bf <+63>:    mov    $0x1,%eax
>    0x004166c4 <+68>:    mov    %eax,0x4(%esp)
>    0x004166c8 <+72>:    mov    %ecx,0x8(%esp)
>    0x004166cc <+76>:    call   0x416ad0 <runtime.stdcall>
>    0x004166d1 <+81>:    add    $0xc,%esp
>    0x004166d4 <+84>:    ret
> End of assembler dump.
> (gdb) info r
> eax            0x1      1
> ecx            0x0      0
> edx            0x0      0
> ebx            0x0      0
> esp            0x3182fecc       0x3182fecc
> ebp            0x3182ff30       0x3182ff30
> esi            0xa      10
> edi            0x0      0
> eip            0x416687 0x416687 <runtime.usleep+7>
> eflags         0x10246  [ PF ZF IF RF ]
> cs             0x1b     27
> ss             0x23     35
> ds             0x23     35
> es             0x23     35
> fs             0x3b     59
> gs             0x0      0
> (gdb) bt
> #0  0x00416687 in runtime.usleep (us=void)
>     at C:/go/root/src/pkg/runtime/thread_windows.c:146
> #1  0x004121bd in lockextra (nilokay=void)
>     at C:/go/root/src/pkg/runtime/proc.c:1044
> #2  0x00411fbc in runtime.needm (x=void)
>     at C:/go/root/src/pkg/runtime/proc.c:915
> #3  0x0041c70c in runtime.cgocallback (fn=void, frame=void,
> framesize=void)
>     at C:/go/root/src/pkg/runtime/asm_386.s:495
> #4  0x0042c780 in _cgoexp_e9125f37873d_Add (a=void, n=void)
>     at
>
C:/DOCUME~1/brainman/LOCALS~1/Temp/go-build908326399/_/C_/go/root/misc/cgo/test/_test/_cgo_defun.c:321
> #5  0x00480836 in ?? ()
> #6  0x0042ccdb in ?? ()
> #7  0x0042d65c in ?? ()
> #8  0x77c3a3b0 in msvcrt!_endthreadex () from
> C:\WINDOWS\system32\msvcrt.dll
> #9  0x7c80b729 in KERNEL32!GetModuleFileNameA ()
>    from C:\WINDOWS\system32\kernel32.dll
> #10 0x00000000 in ?? ()
> (gdb)
>
>
> https://codereview.appspot.com/7304104/

[brainman, 2013-02-21 06:02:19]

On 2013/02/21 05:04:32, dvyukov wrote:
> I guess we need to mark usleep/osyield as #pragma textflag,7 (no split stack).
> 

Does not help. Same problem at different place:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 6988.0x12c4]
runtime.asmcgocall (fn=void, arg=void)
    at c:/go/root/src/pkg/runtime/asm_386.s:450
450             MOVL    m(CX), BP
(gdb) disas
Dump of assembler code for function runtime.asmcgocall:
   0x0041c670 <+0>:     mov    0x4(%esp),%eax
   0x0041c674 <+4>:     mov    0x8(%esp),%ebx
   0x0041c678 <+8>:     mov    %esp,%edx
   0x0041c67a <+10>:    mov    %fs:0x14,%ecx
=> 0x0041c681 <+17>:    mov    0x4(%ecx),%ebp
   0x0041c684 <+20>:    mov    0x0(%ebp),%esi
   0x0041c687 <+23>:    mov    (%ecx),%edi
   0x0041c689 <+25>:    cmp    %edi,%esi
   0x0041c68b <+27>:    je     0x41c69f <runtime.asmcgocall+47>
   0x0041c68d <+29>:    mov    %esp,0x10(%edi)
   0x0041c690 <+32>:    movl   $0x41c660,0x14(%edi)
   0x0041c697 <+39>:    mov    %edi,0x18(%edi)
   0x0041c69a <+42>:    mov    %esi,(%ecx)
   0x0041c69c <+44>:    mov    0x10(%esi),%esp
   0x0041c69f <+47>:    sub    $0x20,%esp
   0x0041c6a2 <+50>:    and    $0xfffffff0,%esp
   0x0041c6a5 <+53>:    mov    %edi,0x8(%esp)
   0x0041c6a9 <+57>:    mov    %edx,0x4(%esp)
   0x0041c6ad <+61>:    mov    %ebx,(%esp)
   0x0041c6b0 <+64>:    call   *%eax
   0x0041c6b2 <+66>:    mov    %fs:0x14,%ecx
   0x0041c6b9 <+73>:    mov    0x8(%esp),%edi
   0x0041c6bd <+77>:    mov    %edi,(%ecx)
   0x0041c6bf <+79>:    mov    0x4(%esp),%esp
   0x0041c6c3 <+83>:    ret
End of assembler dump.
(gdb) info r
eax            0x41c8e0 4311264
ecx            0x0      0
edx            0x3192fe98       831717016
ebx            0x3192fea4       831717028
esp            0x3192fe98       0x3192fe98
ebp            0x3192ff30       0x3192ff30
esi            0xa      10
edi            0x0      0
eip            0x41c681 0x41c681 <runtime.asmcgocall+17>
eflags         0x10206  [ PF IF RF ]
cs             0x1b     27
ss             0x23     35
ds             0x23     35
es             0x23     35
fs             0x3b     59
gs             0x0      0
(gdb) bt
#0  runtime.asmcgocall (fn=void, arg=void)
    at c:/go/root/src/pkg/runtime/asm_386.s:450
#1  0x00416ae0 in runtime.stdcall (fn=void, count=void)
    at c:/go/root/src/pkg/runtime/thread_windows.c:257
#2  0x004166b8 in runtime.usleep (us=void)
    at c:/go/root/src/pkg/runtime/thread_windows.c:152
#3  0x004121bd in lockextra (nilokay=void)
    at c:/go/root/src/pkg/runtime/proc.c:1044
#4  0x00411fbc in runtime.needm (x=void)
    at c:/go/root/src/pkg/runtime/proc.c:915
#5  0x0041c6ec in runtime.cgocallback (fn=void, frame=void, framesize=void)
    at c:/go/root/src/pkg/runtime/asm_386.s:495
#6  0x0042c760 in _cgoexp_15d16e9ca10b_Add (a=void, n=void)
    at
C:/DOCUME~1/brainman/LOCALS~1/Temp/go-build788492787/_/C_/go/root/misc/cgo/test/_test/_cgo_defun.c:321
#7  0x00480816 in ?? ()
#8  0x0042ccbb in ?? ()
#9  0x0042d63c in ?? ()
#10 0x77c3a3b0 in msvcrt!_endthreadex () from C:\WINDOWS\system32\msvcrt.dll
#11 0x7c80b729 in KERNEL32!GetModuleFileNameA ()
   from C:\WINDOWS\system32\kernel32.dll
#12 0x00000000 in ?? ()
(gdb)

Alex

[dvyukov, 2013-02-21 15:45:45]

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

https://codereview.appspot.com/7304104/diff/44001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:944: runtime·minit();
part of the problem was here
minit() allocated gsignal, but we've not yet called exitsyscall() so we are
potentially running concurrently with GC

it seems to be fixed by mpreinit() patch
but I still observe some crashes on linux on misc/cgo/test
still looks like corrupted heap

[dvyukov, 2013-02-21 16:40:28]

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

https://codereview.appspot.com/7304104/diff/44001/src/pkg/runtime/proc.c#newc...
src/pkg/runtime/proc.c:944: runtime·minit();
On 2013/02/21 15:45:45, dvyukov wrote:
> part of the problem was here
> minit() allocated gsignal, but we've not yet called exitsyscall() so we are
> potentially running concurrently with GC
> 
> it seems to be fixed by mpreinit() patch
> but I still observe some crashes on linux on misc/cgo/test
> still looks like corrupted heap

I've noticed that if I set GOGC=off the crash goes away, and if I set GOGC=1 it
crashes more reliably.
It probably means that we either allocate some memory concurrently with GC or
some memory is not reachable.

[dvyukov, 2013-02-21 16:55:08]

On 2013/02/21 16:40:28, dvyukov wrote:
> https://codereview.appspot.com/7304104/diff/44001/src/pkg/runtime/proc.c
> File src/pkg/runtime/proc.c (right):
> 
>
https://codereview.appspot.com/7304104/diff/44001/src/pkg/runtime/proc.c#newc...
> src/pkg/runtime/proc.c:944: runtime·minit();
> On 2013/02/21 15:45:45, dvyukov wrote:
> > part of the problem was here
> > minit() allocated gsignal, but we've not yet called exitsyscall() so we are
> > potentially running concurrently with GC
> > 
> > it seems to be fixed by mpreinit() patch
> > but I still observe some crashes on linux on misc/cgo/test
> > still looks like corrupted heap
> 
> I've noticed that if I set GOGC=off the crash goes away, and if I set GOGC=1
it
> crashes more reliably.
> It probably means that we either allocate some memory concurrently with GC or
> some memory is not reachable.

OK, mystery solved. I will send a CL.