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

[r...@golang.org]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@googlegroups.com,

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


Description:
runtime: allow cgo callbacks on non-Go threads

Fixes issue 4435.

Please review this at https://codereview.appspot.com/7304104/

Affected files:
M misc/cgo/test/cgo_test.go
A misc/cgo/test/cthread.c
A misc/cgo/test/cthread.go
M src/pkg/runtime/asm_386.s
M src/pkg/runtime/asm_amd64.s
M src/pkg/runtime/asm_arm.s
M src/pkg/runtime/cgocall.c
M src/pkg/runtime/proc.c
M src/pkg/runtime/runtime.h
M src/pkg/runtime/thread_plan9.c
M src/pkg/runtime/thread_windows.c

[ia...@golang.org]

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#newcode28
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#newcode28
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#newcode498
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#newcode531
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#newcode597
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#newcode220
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#newcode998
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.

https://codereview.appspot.com/7304104/

[alex.b...@gmail.com]

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_windows.c
File src/pkg/runtime/thread_windows.c (right):

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

https://codereview.appspot.com/7304104/

[alex.b...@gmail.com]

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

Should do the same to thread_plan9.c.

Alex

https://codereview.appspot.com/7304104/

[minu...@gmail.com]

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#newcode376
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#newcode904
src/pkg/runtime/proc.c:904: // is - once it has installed its own m so
that ican do things like
s/ican/it can/

https://codereview.appspot.com/7304104/

[r...@golang.org]

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#newcode28
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#newcode28
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#newcode498
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#newcode531
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#newcode597
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#newcode376
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#newcode220
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#newcode904
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#newcode998
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_windows.c
File src/pkg/runtime/thread_windows.c (right):

https://codereview.appspot.com/7304104/diff/2001/src/pkg/runtime/thread_windows.c#newcode217
src/pkg/runtime/thread_windows.c:217: {

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

Done.

https://codereview.appspot.com/7304104/

[alex.b...@gmail.com]

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

https://codereview.appspot.com/7304104/

[Ian Lance Taylor]

On Tue, Feb 19, 2013 at 12:59 PM, <r...@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

[ia...@golang.org]

LGTM

https://codereview.appspot.com/7304104/

[dvy...@google.com]

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#newcode21
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#newcode32
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#newcode33
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#newcode807
src/pkg/runtime/proc.c:807: runtime·newextram();
if(runtime·iscgo)?

https://codereview.appspot.com/7304104/diff/6002/src/pkg/runtime/proc.c#newcode940
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#newcode945
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#newcode1004
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#newcode1011
src/pkg/runtime/proc.c:1011: runtime·setprof(false);
It's already disabled by entersyscall.

https://codereview.appspot.com/7304104/

[Russ Cox]

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

[Russ Cox]

On Tue, Feb 19, 2013 at 7:21 PM, <alex.b...@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

[r...@golang.org]

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#newcode807
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#newcode940
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#newcode945
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#newcode1004
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#newcode1011
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.

https://codereview.appspot.com/7304104/

[dvy...@google.com]

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


https://codereview.appspot.com/7304104/diff/6002/src/pkg/runtime/proc.c#newcode940
> 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#newcode945
> 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...


https://codereview.appspot.com/7304104/

[Russ Cox]

On Wed, Feb 20, 2013 at 10:31 AM, <dvy...@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

[Russ Cox]

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

[Russ Cox]

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

[alex.b...@gmail.com]

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

Alex

https://codereview.appspot.com/7304104/

[r...@golang.org]

*** 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


https://codereview.appspot.com/7304104/

[Russ Cox]

On Wed, Feb 20, 2013 at 5:43 PM, <alex.b...@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

[alex.b...@gmail.com]

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/

[Dmitry Vyukov]

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

[alex.b...@gmail.com]

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

https://codereview.appspot.com/7304104/

[dvy...@google.com]

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#newcode944
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

https://codereview.appspot.com/7304104/

[dvy...@google.com]

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#newcode944
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.

https://codereview.appspot.com/7304104/

[dvy...@google.com]

OK, mystery solved. I will send a CL.


https://codereview.appspot.com/7304104/