code review 135830043: cmd/cc, runtime: convert C compilers to use Go calling ...

cmd/cc, runtime: convert C compilers to use Go calling convention

To date, the C compilers and Go compilers differed only in how
values were returned from functions. This made it difficult to call
Go from C or C from Go if return values were involved. It also made
assembly called from Go and assembly called from C different.

This CL changes the C compiler to use the Go conventions, passing
results on the stack, after the arguments.
[Exception: this does not apply to C ... functions, because you can't
know where on the stack the arguments end.]

By doing this, the CL makes it possible to rewrite C functions into Go
one at a time, without worrying about which languages call that
function or which languages it calls.

This CL also updates all the assembly files in package runtime to use
the new conventions. Argument references of the form 40(SP) have
been rewritten to the form name+10(FP) instead, and there are now
Go func prototypes for every assembly function called from C or Go.
This means that 'go vet runtime' checks effectively every assembly
function, and go vet's output was used to automate the bulk of the
conversion.

Some functions, like seek and nsec on Plan 9, needed to be rewritten.

Many assembly routines called from C were reading arguments
incorrectly, using MOVL instead of MOVQ or vice versa, especially on
the less used systems like openbsd.
These were found by go vet and have been corrected too.
If we're lucky, this may reduce flakiness on those systems.

Tested on:
        darwin/386
        darwin/amd64
        linux/arm
        linux/386
        linux/amd64
If this breaks another system, the bug is almost certainly in the
sys_$GOOS_$GOARCH.s file, since the rest of the CL is tested
by the combination of the above systems.

[rsc, 2014-08-27 05:03:47]

Hello golang-codereviews@googlegroups.com (cc: dfc, iant, josharian, r),

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

[0intro, 2014-08-27 05:25:21]

It seems runtime.seek is failing on Plan 9.

cmd/go
runtime: panic before malloc heap initialized
fatal error: cas3
go_bootstrap 422461: suicide: invalid address 0x0/8 in sys call pc=0x6a7a7
go_bootstrap 422461: suicide: sys: bad address in syscall pc=0x0006a7a7
cpu% acid 422461
/proc/422461/text:386 plan 9 executable
/sys/lib/acid/port
/sys/lib/acid/386
acid: stk()
runtime.seek()+0x7 ?file?:0
runtime.getenv()+0x10b ?file?:0
runtime.gotraceback()+0x8b ?file?:0
runtime.dopanic()+0xa3 ?file?:0
runtime.throw()+0x74 ?file?:0
runtime.check()+0xee ?file?:0
_rt0_go+0xce ?file?:0
acid: asm(*PC)
runtime.seek+0x7 0x0006a7a7	CMPL	AX,$0xffffffff
runtime.seek+0xa 0x0006a7aa	JNE	runtime.seek+0x15(SB)
runtime.seek+0xc 0x0006a7ac	MOVL	0x4(FP),CX
runtime.seek+0x10 0x0006a7b0	MOVL	AX,0x0(CX)
runtime.seek+0x12 0x0006a7b2	MOVL	AX,0x4(CX)
runtime.seek+0x15 0x0006a7b5	RET
runtime.seek+0x16 0x0006a7b6	ADDB	AL,0x0(AX)
runtime.seek+0x18 0x0006a7b8	ADDB	AL,0x0(AX)
runtime.seek+0x1a 0x0006a7ba	ADDB	AL,0x0(AX)
runtime.seek+0x1c 0x0006a7bc	ADDB	AL,0x0(AX)
runtime.seek+0x1e 0x0006a7be	ADDB	AL,0x0(AX)
runtime.close 0x0006a7c0	MOVL	$0x4,AX
acid: regs()
PC	0x0006a7a7 runtime.seek+0x7  ?file?:0
SP	0xdfffed74 ECODE 0xf010083e EFLAG 0x00000246
CS	0x00000023 DS	 0x0000001b SS	0x0000001b
GS	0x0000001b FS	 0x0000001b ES	0x0000001b
TRAP	0x00000040 system call
AX	0xffffffff BX	0x0000000b CX	0x00000002 DX	0x00000000
DI	0xdfffed99 SI	0x0030c2a0 BP	0x4b434142

[dave_cheney.net, 2014-08-27 05:43:43]

Solaris reports we've run out of stack in chansend1

runtime.chansend1: nosplit stack overflow
        120     assumed on entry to runtime.chansend1
        72      after runtime.chansend1 uses 48
        64      on entry to runtime.chansend
        128     after runtime.chansend uses -64
        120     on entry to runtime.gopark
        128     after runtime.gopark uses -8
        120     on entry to runtime.gothrow
        128     after runtime.gothrow uses -8
        120     on entry to runtime.startpanic
        128     after runtime.startpanic uses -8
        120     on entry to runtime.dopanic
        128     after runtime.dopanic uses -8
        120     on entry to runtime.tracebackothers
        128     after runtime.tracebackothers uses -8
        120     on entry to runtime.lock
        128     after runtime.lock uses -8
        120     on entry to runtime.semacreate
        88      after runtime.semacreate uses 32
        80      on entry to runtime.sem_init
        40      after runtime.sem_init uses 40
        32      on entry to runtime.sysvicall6
        16      after runtime.sysvicall6 uses 16
        8       on entry to runtime.asmcgocall
        0       on entry to function pointer
        -8      on entry to runtime.morestack
runtime.needm: nosplit stack overflow
        120     assumed on entry to runtime.needm
        80      after runtime.needm uses 40
        72      on entry to lockextra
        24      after lockextra uses 48
        16      on entry to runtime.usleep
        8       after runtime.usleep uses 8
        0       on entry to runtime.usleep1
        -8      on entry to function pointer
        -16     on entry to runtime.morestack
go tool dist: FAILED: /home/dfc/go/pkg/tool/solaris_amd64/6l -o
/home/dfc/go/pkg/tool/solaris_amd64/go_bootstrap $WORK/_go_.a

[dave_cheney.net, 2014-08-27 06:08:26]

linux/arm is happen with this change.

Thanks for unifying vlrt_{386,arm}.c

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/sys_linu...
File src/pkg/runtime/sys_linux_amd64.s (right):

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/sys_linu...
src/pkg/runtime/sys_linux_amd64.s:298: MOVL	AX, ret+40(FP)
the comment says this returns an int64, which is correct ?

[brainman, 2014-08-27 07:00:21]

This is broken on windows in a multiple wonderful ways. :-)

Alex

[dvyukov, 2014-08-27 09:03:29]

Whoa! This is cool!

Reasons for different calling conventions are pure historical, right?

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go
File src/pkg/runtime/stubs.go (right):

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
src/pkg/runtime/stubs.go:223: func gogo(buf unsafe.Pointer)
s/unsafe.Pointer/*gobuf/

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
src/pkg/runtime/stubs.go:224: func gosave(buf unsafe.Pointer)
s/unsafe.Pointer/*gobuf/

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
src/pkg/runtime/stubs.go:229: func mincore(addr unsafe.Pointer, n uintptr, dst
*byte) int32
move to sys_foo.go files, it's not super portable

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
src/pkg/runtime/stubs.go:249: func setg(gg *g)
s/gg/gp/

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
src/pkg/runtime/stubs.go:258: func madvise(addr unsafe.Pointer, n uintptr, flags
int32)
move mmap/munmap/madvise to sys_foo.go files

[rsc, 2014-08-27 11:14:29]

I'm trying to avoid making ANY more changes here. I will fix Windows and Solaris
and any other breakages after submitting the CL. It's gotten too big to move
around at this point, and the breakages won't be bad. I've tested on 5 different
setups so I know it's mostly right.

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go
File src/pkg/runtime/stubs.go (right):

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
src/pkg/runtime/stubs.go:223: func gogo(buf unsafe.Pointer)
On 2014/08/27 09:03:28, dvyukov wrote:
> s/unsafe.Pointer/*gobuf/

Done.

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
src/pkg/runtime/stubs.go:224: func gosave(buf unsafe.Pointer)
On 2014/08/27 09:03:28, dvyukov wrote:
> s/unsafe.Pointer/*gobuf/

Done.

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
src/pkg/runtime/stubs.go:229: func mincore(addr unsafe.Pointer, n uintptr, dst
*byte) int32
On 2014/08/27 09:03:28, dvyukov wrote:
> move to sys_foo.go files, it's not super portable

No, not in this change. It's in runtime.h, so it's here. Maybe later we can do
cleanup, but not now.

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
src/pkg/runtime/stubs.go:249: func setg(gg *g)
On 2014/08/27 09:03:29, dvyukov wrote:
> s/gg/gp/

No, not in this change. Changing any of these names will require touching the
assembly files again to make 'go vet' happy. It's fine to do later.

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
src/pkg/runtime/stubs.go:258: func madvise(addr unsafe.Pointer, n uintptr, flags
int32)
On 2014/08/27 09:03:28, dvyukov wrote:
> move mmap/munmap/madvise to sys_foo.go files

No. (See above.)

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/sys_linu...
File src/pkg/runtime/sys_linux_amd64.s (right):

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/sys_linu...
src/pkg/runtime/sys_linux_amd64.s:298: MOVL	AX, ret+40(FP)
On 2014/08/27 06:08:26, dfc wrote:
> the comment says this returns an int64, which is correct ?

I fixed the comment (but haven't reuploaded).

os_linux.go is enforced upon the assembly by 'go vet', the assembly is correct
as long as the prototypes are and 'go vet' is happy.

And os_linux.h matches os_linux.go (I just checked), so it really is int32, at
least as far as the C code has always been concerned. Maybe we're throwing away
high bits but I don't think Linux has 32-bit pids yet, much less 64-bit ones.

[dave_cheney.net, 2014-08-27 11:17:16]

On Wed, Aug 27, 2014 at 9:14 PM,  <rsc@golang.org> wrote:
> I'm trying to avoid making ANY more changes here. I will fix Windows and
> Solaris and any other breakages after submitting the CL. It's gotten too
> big to move around at this point, and the breakages won't be bad. I've
> tested on 5 different setups so I know it's mostly right.

SGTM.

> https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go
> File src/pkg/runtime/stubs.go (right):
>
>
https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
> src/pkg/runtime/stubs.go:223: func gogo(buf unsafe.Pointer)
> On 2014/08/27 09:03:28, dvyukov wrote:
>>
>> s/unsafe.Pointer/*gobuf/
>
>
> Done.
>
>
>
https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
> src/pkg/runtime/stubs.go:224: func gosave(buf unsafe.Pointer)
> On 2014/08/27 09:03:28, dvyukov wrote:
>>
>> s/unsafe.Pointer/*gobuf/
>
>
> Done.
>
>
>
https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
> src/pkg/runtime/stubs.go:229: func mincore(addr unsafe.Pointer, n
> uintptr, dst *byte) int32
> On 2014/08/27 09:03:28, dvyukov wrote:
>>
>> move to sys_foo.go files, it's not super portable
>
>
> No, not in this change. It's in runtime.h, so it's here. Maybe later we
> can do cleanup, but not now.
>
>
>
https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
> src/pkg/runtime/stubs.go:249: func setg(gg *g)
> On 2014/08/27 09:03:29, dvyukov wrote:
>>
>> s/gg/gp/
>
>
> No, not in this change. Changing any of these names will require
> touching the assembly files again to make 'go vet' happy. It's fine to
> do later.
>
>
>
https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
> src/pkg/runtime/stubs.go:258: func madvise(addr unsafe.Pointer, n
> uintptr, flags int32)
> On 2014/08/27 09:03:28, dvyukov wrote:
>>
>> move mmap/munmap/madvise to sys_foo.go files
>
>
> No. (See above.)
>
>
>
https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/sys_linu...
> File src/pkg/runtime/sys_linux_amd64.s (right):
>
>
https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/sys_linu...
> src/pkg/runtime/sys_linux_amd64.s:298: MOVL     AX, ret+40(FP)
> On 2014/08/27 06:08:26, dfc wrote:
>>
>> the comment says this returns an int64, which is correct ?
>
>
> I fixed the comment (but haven't reuploaded).
>
> os_linux.go is enforced upon the assembly by 'go vet', the assembly is
> correct as long as the prototypes are and 'go vet' is happy.
>
> And os_linux.h matches os_linux.go (I just checked), so it really is
> int32, at least as far as the C code has always been concerned. Maybe
> we're throwing away high bits but I don't think Linux has 32-bit pids
> yet, much less 64-bit ones.

man 2 clone says there are actually two clone calls, the latter, the
raw clone returns a linux long, which I guess is an int64. I think
it's fine to cull those upper bits, as you point out, linux doesn't
support pids in the billions.

[rsc, 2014-08-27 11:22:39]

On Wed, Aug 27, 2014 at 5:03 AM, <dvyukov@google.com> wrote:

> Whoa! This is cool!
>
> Reasons for different calling conventions are pure historical, right?
>

Yes. The C compiler originally did what most C compilers did: all arguments
are first promoted at least to int, and the return value is in a register.
When C came to grips with large return values, the usual fix was to pass a
"fill this in" pointer as the first argument (in general you can't be 100%
sure where the caller thinks the input arguments end, due to unenforced
prototypes). Go made different choices for alignment of arguments (not
everything is promoted to int like in C) and for return values (on the
stack, to support multiple or large return values).

When we started doing more "Go calling C" things, I changed the C compilers
so that the arguments passed on the stack lined up the same way as in Go,
but I never made return values match up. For "Go calling C" we could do
return values using the FLUSH hack, but for "C calling Go" there was no
way. The conversion made it more important to fix the compiler.

Russ

[rsc, 2014-08-27 11:23:35]

On Wed, Aug 27, 2014 at 7:17 AM, Dave Cheney <dave@cheney.net> wrote:

> man 2 clone says there are actually two clone calls, the latter, the
> raw clone returns a linux long, which I guess is an int64. I think
> it's fine to cull those upper bits, as you point out, linux doesn't
> support pids in the billions.
>

Does it support pids >= 32768 yet?

Russ

[dave_cheney.net, 2014-08-27 11:26:25]

On Wed, Aug 27, 2014 at 9:23 PM, Russ Cox <rsc@golang.org> wrote:
> On Wed, Aug 27, 2014 at 7:17 AM, Dave Cheney <dave@cheney.net> wrote:
>>
>> man 2 clone says there are actually two clone calls, the latter, the
>> raw clone returns a linux long, which I guess is an int64. I think
>> it's fine to cull those upper bits, as you point out, linux doesn't
>> support pids in the billions.
>
>
> Does it support pids >= 32768 yet?

I *think* so, from Kernel 3.0 onwards, but I haven't been following
LKML for a long time.

>
> Russ

[dvyukov, 2014-08-27 11:29:21]

forget to hg upload

[rsc, 2014-08-27 11:31:23]

On Wed, Aug 27, 2014 at 7:29 AM, <dvyukov@google.com> wrote:

> forget to hg upload


i didn't forget. i made the two minor changes you asked for but i do not
want to upload a new copy until i am submitting. the CL is so huge want
everyone commenting on the same revision.

[dvyukov, 2014-08-27 11:39:59]

LGTM

the code as-is requires some cleanups, but they will be done in separate changes

I am fine if fixes of breakages of some OSes will be done separately one-by-one

[iant, 2014-08-27 13:01:58]

LGTM

I tried to look at everything but there is really too much here to grasp.  I
guess we'll find issues as we go forward.

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go
File src/pkg/runtime/stubs.go (right):

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/stubs.go...
src/pkg/runtime/stubs.go:226: func read(fd int32, p unsafe.Pointer, n int32)
int32
Making n and the return type int32 for read and write isn't really right.  On
64-bit systems they should be int64.  Nothing to do in this CL, though.

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/vlop_386.s
File src/pkg/runtime/vlop_386.s (right):

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/vlop_386...
src/pkg/runtime/vlop_386.s:45: MOVL	DX, AX
I think just
MOVL DX, ret+16(FP)
No obvious need to go through AX.

[rsc, 2014-08-27 15:25:21]

On Wed, Aug 27, 2014 at 9:01 AM, <iant@golang.org> wrote:

> LGTM
>
> I tried to look at everything but there is really too much here to
> grasp.  I guess we'll find issues as we go forward.


Yes, sorry. For what it's worth, the vast majority of this was written by
my programs, not by me, and I do believe I read every line.

Russ

[rsc, 2014-08-27 15:26:20]

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/vlop_386.s
File src/pkg/runtime/vlop_386.s (right):

https://codereview.appspot.com/135830043/diff/140001/src/pkg/runtime/vlop_386...
src/pkg/runtime/vlop_386.s:45: MOVL	DX, AX
On 2014/08/27 13:01:58, iant wrote:
> I think just
> MOVL DX, ret+16(FP)
> No obvious need to go through AX.

True enough. It doesn't hurt (usually), and it was an automated insertion in
functions that needed it.
(Actually this one wasn't automated, since it is not a runtime. function, but I
was being consistent with the many automated ones.)

[rsc, 2014-08-27 15:32:22]

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

cmd/cc, runtime: convert C compilers to use Go calling convention

To date, the C compilers and Go compilers differed only in how
values were returned from functions. This made it difficult to call
Go from C or C from Go if return values were involved. It also made
assembly called from Go and assembly called from C different.

This CL changes the C compiler to use the Go conventions, passing
results on the stack, after the arguments.
[Exception: this does not apply to C ... functions, because you can't
know where on the stack the arguments end.]

By doing this, the CL makes it possible to rewrite C functions into Go
one at a time, without worrying about which languages call that
function or which languages it calls.

This CL also updates all the assembly files in package runtime to use
the new conventions. Argument references of the form 40(SP) have
been rewritten to the form name+10(FP) instead, and there are now
Go func prototypes for every assembly function called from C or Go.
This means that 'go vet runtime' checks effectively every assembly
function, and go vet's output was used to automate the bulk of the
conversion.

Some functions, like seek and nsec on Plan 9, needed to be rewritten.

Many assembly routines called from C were reading arguments
incorrectly, using MOVL instead of MOVQ or vice versa, especially on
the less used systems like openbsd.
These were found by go vet and have been corrected too.
If we're lucky, this may reduce flakiness on those systems.

Tested on:
        darwin/386
        darwin/amd64
        linux/arm
        linux/386
        linux/amd64
If this breaks another system, the bug is almost certainly in the
sys_$GOOS_$GOARCH.s file, since the rest of the CL is tested
by the combination of the above systems.

LGTM=dvyukov, iant
R=golang-codereviews, 0intro, dave, alex.brainman, dvyukov, iant
CC=golang-codereviews, josharian, r
https://codereview.appspot.com/135830043

[gobot, 2014-08-27 15:33:56]

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

[aram2, 2014-08-27 19:05:47]

Great.

LGTM

-- 
Aram Hăvărneanu