code review 138740043: cmd/cc, runtime: preserve C runtime type names in gener...

cmd/cc, runtime: preserve C runtime type names in generated Go

uintptr or uint64 in the runtime C were turning into uint in the Go,
bool was turning into uint8, and so on. Fix that.

Also delete Go wrappers for C functions.
The C functions can be called directly now
(but still eventually need to be converted to Go).

[rsc, 2014-08-27 19:52:44]

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

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

[bradfitz, 2014-08-27 20:36:59]

LGTM

https://codereview.appspot.com/138740043/diff/40001/src/cmd/cc/sub.c
File src/cmd/cc/sub.c (right):

https://codereview.appspot.com/138740043/diff/40001/src/cmd/cc/sub.c#newcode1209
src/cmd/cc/sub.c:1209: if(nerrors > 1000) {
leftover debugging?

https://codereview.appspot.com/138740043/diff/40001/src/cmd/cc/sub.c#newcode1252
src/cmd/cc/sub.c:1252: if(nerrors > 1000) {
leftover debugging?

[bradfitz, 2014-08-27 20:37:44]

Sorry, comments were correct, but LGTMed the wrong CL.

[bradfitz, 2014-08-27 20:39:31]

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/arch_386.go
File src/pkg/runtime/arch_386.go (right):

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/arch_386....
src/pkg/runtime/arch_386.go:12: type intptr int32 // TODO(rsc): remove
when?

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/arch_amd6...
File src/pkg/runtime/arch_amd64.go (right):

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/arch_amd6...
src/pkg/runtime/arch_amd64.go:12: type intptr int64 // TODO(rsc): remove
when?

[iant, 2014-08-27 20:42:38]

https://codereview.appspot.com/138740043/diff/40001/src/cmd/cc/sub.c
File src/cmd/cc/sub.c (right):

https://codereview.appspot.com/138740043/diff/40001/src/cmd/cc/sub.c#newcode1209
src/cmd/cc/sub.c:1209: if(nerrors > 1000) {
Should the changes to this file be part of this CL?

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/iface.go
File src/pkg/runtime/iface.go (right):

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/iface.go#...
src/pkg/runtime/iface.go:79: m =
(*itab)(persistentalloc(unsafe.Sizeof(itab{})+uintptr(len(inter.mhdr))*ptrSize,
0, (*uint64)(unsafe.Pointer(&memstats.other_sys))))
If the godefs change works, I think the other_sys field should have type uint64
anyhow.  Why is this type conversion needed?

[rsc, 2014-08-27 20:42:59]

https://codereview.appspot.com/138740043/diff/40001/src/cmd/cc/sub.c
File src/cmd/cc/sub.c (right):

https://codereview.appspot.com/138740043/diff/40001/src/cmd/cc/sub.c#newcode1252
src/cmd/cc/sub.c:1252: if(nerrors > 1000) {
On 2014/08/27 20:36:59, bradfitz wrote:
> leftover debugging?

Took file out, thanks.

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/arch_386.go
File src/pkg/runtime/arch_386.go (right):

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/arch_386....
src/pkg/runtime/arch_386.go:12: type intptr int32 // TODO(rsc): remove
On 2014/08/27 20:39:31, bradfitz wrote:
> when?

when i have removed the uses of it.
it's usually a bad idea.

[minux, 2014-08-28 00:57:16]

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

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/asm_amd64...
src/pkg/runtime/asm_amd64.s:624: TEXT runtime·casuintptr(SB), NOSPLIT, $0-13
s/13/25/?

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

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/asm_arm.s...
src/pkg/runtime/asm_arm.s:674: TEXT runtime·armcas(SB),NOSPLIT,$0-12
this function is not using the Go func call convention now.
fix it (probably in another CL)?

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/iface.go
File src/pkg/runtime/iface.go (right):

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/iface.go#...
src/pkg/runtime/iface.go:79: m =
(*itab)(persistentalloc(unsafe.Sizeof(itab{})+uintptr(len(inter.mhdr))*ptrSize,
0, (*uint64)(unsafe.Pointer(&memstats.other_sys))))
On 2014/08/27 20:42:38, iant wrote:
> If the godefs change works, I think the other_sys field should have type
uint64
> anyhow.  Why is this type conversion needed?
I wonder the same.

[rsc, 2014-08-28 01:09:30]

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/arch_amd6...
File src/pkg/runtime/arch_amd64.go (right):

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/arch_amd6...
src/pkg/runtime/arch_amd64.go:12: type intptr int64 // TODO(rsc): remove
On 2014/08/27 20:39:31, bradfitz wrote:
> when?

when i remove it from the code that uses it.

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

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/asm_amd64...
src/pkg/runtime/asm_amd64.s:624: TEXT runtime·casuintptr(SB), NOSPLIT, $0-13
On 2014/08/28 00:57:15, minux wrote:
> s/13/25/?

Done.

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

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/asm_arm.s...
src/pkg/runtime/asm_arm.s:674: TEXT runtime·armcas(SB),NOSPLIT,$0-12
On 2014/08/28 00:57:15, minux wrote:
> this function is not using the Go func call convention now.
> fix it (probably in another CL)?

Done.

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/iface.go
File src/pkg/runtime/iface.go (right):

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/iface.go#...
src/pkg/runtime/iface.go:79: m =
(*itab)(persistentalloc(unsafe.Sizeof(itab{})+uintptr(len(inter.mhdr))*ptrSize,
0, (*uint64)(unsafe.Pointer(&memstats.other_sys))))
On 2014/08/27 20:42:38, iant wrote:
> If the godefs change works, I think the other_sys field should have type
uint64
> anyhow.  Why is this type conversion needed?

You're right, sorry. Originally it was (*uint), which prompted the change, and
then I fixed the compile error by updating the type instead of simplifying. Will
fix.

https://codereview.appspot.com/138740043/diff/40001/src/pkg/runtime/iface.go#...
src/pkg/runtime/iface.go:79: m =
(*itab)(persistentalloc(unsafe.Sizeof(itab{})+uintptr(len(inter.mhdr))*ptrSize,
0, (*uint64)(unsafe.Pointer(&memstats.other_sys))))
On 2014/08/28 00:57:15, minux wrote:
> On 2014/08/27 20:42:38, iant wrote:
> > If the godefs change works, I think the other_sys field should have type
> uint64
> > anyhow.  Why is this type conversion needed?
> I wonder the same.

I forgot to hit send on my reply to Ian.

[rsc, 2014-08-28 01:09:52]

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

Please take another look.

[minux, 2014-08-28 01:14:27]

LGTM.

[iant, 2014-08-28 01:15:49]

LGTM

[rsc, 2014-08-28 01:59:55]

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

cmd/cc, runtime: preserve C runtime type names in generated Go

uintptr or uint64 in the runtime C were turning into uint in the Go,
bool was turning into uint8, and so on. Fix that.

Also delete Go wrappers for C functions.
The C functions can be called directly now
(but still eventually need to be converted to Go).

LGTM=bradfitz, minux, iant
R=golang-codereviews, bradfitz, iant, minux
CC=golang-codereviews, khr, r
https://codereview.appspot.com/138740043

[gobot, 2014-08-28 02:01:06]

This CL appears to have broken the android-arm-crawshaw builder.
See http://build.golang.org/log/83efa450c5089f1af864e3014294f888cb78731d

[dave_cheney.net, 2014-08-28 02:01:52]

This is real



# Building packages and commands for android/arm.
runtime
# runtime
pkg/runtime/asm_arm.s:689 syntax error, last name: runtime.cas
Build complete, duration 26.528922956s. Result: error: exit status 2

On Thu, Aug 28, 2014 at 12:01 PM,  <gobot@golang.org> wrote:
> This CL appears to have broken the android-arm-crawshaw builder.
> See http://build.golang.org/log/83efa450c5089f1af864e3014294f888cb78731d
>
>
> https://codereview.appspot.com/138740043/
>
> --
> 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.

[rsc, 2014-08-28 02:06:45]

fixed