code review 36010043: syscall: generate zfiles for GOOS=solaris

syscall: generate zfiles for GOOS=solaris

[aram, 2013-12-10 20:40:25]

Hello golang-dev@googlegroups.com,

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

[minux1, 2013-12-13 06:51:43]

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/zerrors_s...
File src/pkg/syscall/zerrors_solaris_amd64.go (right):

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/zerrors_s...
src/pkg/syscall/zerrors_solaris_amd64.go:749: SIOCADDMULTI                  =
-0x7fdf96cf
is these constants really negative?

i suspect it's caused by a bug in cmd/cgo or gcc's dwarf output.

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/zsysnum_s...
File src/pkg/syscall/zsysnum_solaris_amd64.go (right):

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/zsysnum_s...
src/pkg/syscall/zsysnum_solaris_amd64.go:4: SYS_EXECVE = 59
do we still need these syscall constants given that we are not going
to make direct syscalls?

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/ztypes_so...
File src/pkg/syscall/ztypes_solaris_amd64.go (right):

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/ztypes_so...
src/pkg/syscall/ztypes_solaris_amd64.go:133: X__sin6_src_id uint32
this field name seems inconsistent with others.

I know this is a known limitation of cmd/cgo, I wonder if we should
correct them by hand?

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/ztypes_so...
src/pkg/syscall/ztypes_solaris_amd64.go:289: type IfmaMsghdr [0]byte
is these zero sized type correct?

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/ztypes_so...
src/pkg/syscall/ztypes_solaris_amd64.go:342: type BpfZbuf [0]byte
and this.

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/ztypes_so...
src/pkg/syscall/ztypes_solaris_amd64.go:370: type BpfZbufHeader [0]byte
and this.

[aram, 2013-12-13 11:55:38]

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/zsysnum_s...
File src/pkg/syscall/zsysnum_solaris_amd64.go (right):

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/zsysnum_s...
src/pkg/syscall/zsysnum_solaris_amd64.go:4: SYS_EXECVE = 59
On 2013/12/13 06:51:43, minux wrote:
> do we still need these syscall constants given that we are not going
> to make direct syscalls?

It's required by exec_unix.go:/syscall.Exec. I don't understand why we
have that function. FWIW syscall.Exec doesn't work on Solaris in this
form. My plan is to implement syscall.Exec in terms of port-specific
(generated) syscall.execve rather than in terms of indirect SYS_EXECVE
syscall, and get rid of both SYS_EXECVE and syscall.Syscall on Solaris.

Same thing for SYS_FCNTL.

[aram, 2013-12-13 16:39:32]

Thanks.

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/zerrors_s...
File src/pkg/syscall/zerrors_solaris_amd64.go (right):

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/zerrors_s...
src/pkg/syscall/zerrors_solaris_amd64.go:749: SIOCADDMULTI                  =
-0x7fdf96cf
> is these constants really negative?
> 
> i suspect it's caused by a bug in cmd/cgo or gcc's dwarf output.

It's weird, but it's correct: 

: oos:1; cat a.c
#include <sys/types.h>
#include <sys/socket.h>
#include <sys/sockio.h>
#include <sys/mman.h>
#include <sys/wait.h>
#include <sys/ioctl.h>
#include <net/bpf.h>
#include <net/if.h>
#include <net/if_arp.h>
#include <net/if_types.h>
#include <net/route.h>
#include <netinet/in.h>
#include <termios.h>
#include <netinet/ip.h>
#include <netinet/ip_mroute.h>
#include <stdio.h>

int
main()
{
	signed int x = SIOCADDMULTI;
	printf("C:	0x%x %d\n", SIOCADDMULTI, SIOCADDMULTI);
	printf("Go:	0x%x %d\n", -0x7fdf96cf, -0x7fdf96cf);
	printf("-Hex:	%s0x%x\n", x<0?"-":"", x<0?-(unsigned)x:x);
}
: oos:1; gcc -m64 a.c
: oos:1; a.out
C:	0x80206931 -2145359567
Go:	0x80206931 -2145359567
-Hex:	-0x7fdf96cf
: oos:1; 

http://src.illumos.org/source/xref/illumos-gate/usr/src/uts/common/sys/sockio...

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/ztypes_so...
File src/pkg/syscall/ztypes_solaris_amd64.go (right):

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/ztypes_so...
src/pkg/syscall/ztypes_solaris_amd64.go:133: X__sin6_src_id uint32
On 2013/12/13 06:51:43, minux wrote:
> this field name seems inconsistent with others.
> 
> I know this is a known limitation of cmd/cgo, I wonder if we should
> correct them by hand?

What good would that do?

https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/ztypes_so...
src/pkg/syscall/ztypes_solaris_amd64.go:289: type IfmaMsghdr [0]byte
On 2013/12/13 06:51:43, minux wrote:
> is these zero sized type correct?

No, this structure doesn't exist on Solaris. I'm uncertain what's the best
course of action. Remove it from types_solaris.go? It's not used anywhere in the
tree.

[minux1, 2013-12-14 07:54:21]

On Dec 13, 2013 11:39 AM, <aram@mgk.ro> wrote:
> On 2013/12/13 06:51:43, minux wrote:
>> this field name seems inconsistent with others.
>> I know this is a known limitation of cmd/cgo, I wonder if we should
>> correct them by hand?
> What good would that do?
For consistency with other field names.
>
>
https://codereview.appspot.com/36010043/diff/200001/src/pkg/syscall/ztypes_so...
> src/pkg/syscall/ztypes_solaris_amd64.go:289: type IfmaMsghdr [0]byte
> On 2013/12/13 06:51:43, minux wrote:
>>
>> is these zero sized type correct?
>
> No, this structure doesn't exist on Solaris. I'm uncertain what's the
> best course of action. Remove it from types_solaris.go? It's not used
> anywhere in the tree.
in that case, we should delete them from types_solaris.go.

[gobot, 2013-12-20 16:26:20]

Replacing golang-dev with golang-codereviews.

[aram, 2013-12-21 21:48:45]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[aram, 2013-12-29 17:45:05]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[aram, 2014-01-09 14:18:36]

> in that case, we should delete them from types_solaris.go.

Done, also removed some non-existent system calls.

[aram, 2014-01-09 15:10:40]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[gobot, 2014-01-09 15:13:16]

R=jsing@google.com (assigned by aram@mgk.ro)

[aram, 2014-01-17 16:02:27]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com, gobot@golang.org,
jsing@google.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[aram, 2014-02-12 11:23:58]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com, gobot@golang.org,
jsing@google.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[aram, 2014-02-22 17:18:45]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com, gobot@golang.org,
jsing@google.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[aram, 2014-02-22 17:23:18]

Gentle ping.

[minux1, 2014-02-22 21:54:56]

LGTM. Let's get this in soon.

https://codereview.appspot.com/36010043/diff/550001/src/pkg/syscall/zsysnum_s...
File src/pkg/syscall/zsysnum_solaris_amd64.go (right):

https://codereview.appspot.com/36010043/diff/550001/src/pkg/syscall/zsysnum_s...
src/pkg/syscall/zsysnum_solaris_amd64.go:4: SYS_EXECVE = 59
why we still need these two syscall numbers?
are we still calling syscall directly?

[aram, 2014-02-22 21:59:32]

https://codereview.appspot.com/36010043/diff/550001/src/pkg/syscall/zsysnum_s...
File src/pkg/syscall/zsysnum_solaris_amd64.go (right):

https://codereview.appspot.com/36010043/diff/550001/src/pkg/syscall/zsysnum_s...
src/pkg/syscall/zsysnum_solaris_amd64.go:4: SYS_EXECVE = 59
On 2014/02/22 21:54:57, minux wrote:
> why we still need these two syscall numbers?
> are we still calling syscall directly?

Net uses SYS_FCNTL, syscall uses SYS_EXECVE in exec_unix.go. We should make net
use syscall.Fcntl and move syscall.Exec out of exec_unix.go, although I'm not
sure where to put it.

[mikio, 2014-02-24 00:27:20]

I can't repro making the z-file for system constants on solaris/amd64. Is there
any magical stuff?

oi_151a8:~/go/src/pkg/syscall$ ./mkerrors.sh -m64
// mkerrors.sh -m64
// MACHINE GENERATED BY THE COMMAND ABOVE; DO NOT EDIT
(snip)
        SIGXFSZ                         = Signal( 0x1f)
        SIGXRES                         = Signal( 0x26)
)
_errors.c: In function 'main':
_errors.c:200:9: warning: missing terminating " character [enabled by default]
_errors.c:200:2: error: missing terminating " character
(snip)
_errors.c:249:1: error: invalid use of void expression
_errors.c:249:1: error: expected declaration or statement at end of input

https://codereview.appspot.com/36010043/diff/550001/src/pkg/syscall/zsysnum_s...
File src/pkg/syscall/zsysnum_solaris_amd64.go (right):

https://codereview.appspot.com/36010043/diff/550001/src/pkg/syscall/zsysnum_s...
src/pkg/syscall/zsysnum_solaris_amd64.go:1: package syscall
copyright?

[aram, 2014-02-24 15:44:01]

> I can't repro making the z-file for system constants on solaris/amd64.
> Is there any magical stuff?

Problem is that echo(1) is not POSIX standardised, and Solaris echo(1)
interprets escape sequences itself. I don't remember the magic I used
to generate the z file, perhaps Dave did it.

I have updated mkerrors.sh to work on Solaris, so this works now OOTB,
please review.

https://codereview.appspot.com/36010043/diff/550001/src/pkg/syscall/zsysnum_s...
File src/pkg/syscall/zsysnum_solaris_amd64.go (right):

https://codereview.appspot.com/36010043/diff/550001/src/pkg/syscall/zsysnum_s...
src/pkg/syscall/zsysnum_solaris_amd64.go:1: package syscall
On 2014/02/24 00:27:21, mikio wrote:
> copyright?

Done.

https://codereview.appspot.com/36010043/diff/550001/src/pkg/syscall/zsysnum_s...
src/pkg/syscall/zsysnum_solaris_amd64.go:4: SYS_EXECVE = 59
And yes, this will go away before 1.3.

[aram, 2014-02-24 15:45:13]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com, gobot@golang.org,
jsing@google.com, mikioh.mikioh@gmail.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[dfc, 2014-02-24 20:25:23]

On Tue, Feb 25, 2014 at 2:44 AM, <aram@mgk.ro> wrote:

> I can't repro making the z-file for system constants on solaris/amd64.
>> Is there any magical stuff?
>>
>
> Problem is that echo(1) is not POSIX standardised, and Solaris echo(1)
> interprets escape sequences itself. I don't remember the magic I used
> to generate the z file, perhaps Dave did it.
>

Yup. I used a version of gccgo compiler for solaris to build cmd/go and
cmd/cgo then fed things through cgo -godefs [sic]. It wasn't a very
repeatable process.


>
> I have updated mkerrors.sh to work on Solaris, so this works now OOTB,
> please review.
>
>
>
>
> https://codereview.appspot.com/36010043/diff/550001/src/
> pkg/syscall/zsysnum_solaris_amd64.go
> File src/pkg/syscall/zsysnum_solaris_amd64.go (right):
>
> https://codereview.appspot.com/36010043/diff/550001/src/
> pkg/syscall/zsysnum_solaris_amd64.go#newcode1
> src/pkg/syscall/zsysnum_solaris_amd64.go:1: package syscall
> On 2014/02/24 00:27:21, mikio wrote:
>
>> copyright?
>>
>
> Done.
>
>
> https://codereview.appspot.com/36010043/diff/550001/src/
> pkg/syscall/zsysnum_solaris_amd64.go#newcode4
> src/pkg/syscall/zsysnum_solaris_amd64.go:4: SYS_EXECVE = 59
> And yes, this will go away before 1.3.
>
>
> https://codereview.appspot.com/36010043/
>
> --
> 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/groups/opt_out.
>

[aram, 2014-02-24 21:18:37]

Yeah, well, should be fixed now.

[aram, 2014-02-24 21:19:54]

Hello golang-codereviews@googlegroups.com, minux.ma@gmail.com, gobot@golang.org,
jsing@google.com, mikioh.mikioh@gmail.com, dave@cheney.net (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[mikio, 2014-02-25 07:57:06]

LGTM, leave to minux and/or jsing

https://codereview.appspot.com/36010043/diff/650001/src/pkg/syscall/zsysnum_s...
File src/pkg/syscall/zsysnum_solaris_amd64.go (right):

https://codereview.appspot.com/36010043/diff/650001/src/pkg/syscall/zsysnum_s...
src/pkg/syscall/zsysnum_solaris_amd64.go:4: package syscall
pls insert blank line before package clause

[minux1, 2014-02-25 08:07:32]

LGTM. Mikioh, if it's convenient for you, please just fix the problem and
submit. I won't be able to submit untill later today.

On Feb 25, 2014 2:57 AM, <mikioh.mikioh@gmail.com> wrote:
> LGTM, leave to minux and/or jsing
>
https://codereview.appspot.com/36010043/diff/650001/src/pkg/syscall/zsysnum_s...
> File src/pkg/syscall/zsysnum_solaris_amd64.go (right):
>
>
https://codereview.appspot.com/36010043/diff/650001/src/pkg/syscall/zsysnum_s...
> src/pkg/syscall/zsysnum_solaris_amd64.go:4: package syscall
> pls insert blank line before package clause
>
> https://codereview.appspot.com/36010043/

[mikio, 2014-02-25 08:11:18]

okay, will do soon

On Tue, Feb 25, 2014 at 5:07 PM, minux <minux.ma@gmail.com> wrote:
> LGTM. Mikioh, if it's convenient for you, please just fix the problem and
> submit. I won't be able to submit untill later today.
>
> On Feb 25, 2014 2:57 AM, <mikioh.mikioh@gmail.com> wrote:
>> LGTM, leave to minux and/or jsing
>>
>>
https://codereview.appspot.com/36010043/diff/650001/src/pkg/syscall/zsysnum_s...
>> File src/pkg/syscall/zsysnum_solaris_amd64.go (right):
>>
>>
>>
https://codereview.appspot.com/36010043/diff/650001/src/pkg/syscall/zsysnum_s...
>> src/pkg/syscall/zsysnum_solaris_amd64.go:4: package syscall
>> pls insert blank line before package clause
>>
>> https://codereview.appspot.com/36010043/
>
> --
> 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/groups/opt_out.

[mikio, 2014-02-25 08:12:30]

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

syscall: generate zfiles for GOOS=solaris

LGTM=minux.ma, mikioh.mikioh
R=golang-codereviews, minux.ma, gobot, jsing, mikioh.mikioh, dave
CC=golang-codereviews
https://codereview.appspot.com/36010043

Committer: Mikio Hara <mikioh.mikioh@gmail.com>

[dfc, 2014-02-25 08:19:06]

Oh damn, so close

/home/dfc/work/solaris-amd64-smartos-5f58c6d04b6d/go/src/pkg/syscall/zerrors_solaris_amd64.go:1076:
undefined: Errno
/home/dfc/work/solaris-amd64-smartos-5f58c6d04b6d/go/src/pkg/syscall/zerrors_solaris_amd64.go:1076:
const initializer Errno(7) is not a constant
/home/dfc/work/solaris-amd64-smartos-5f58c6d04b6d/go/src/pkg/syscall/zerrors_solaris_amd64.go:1077:
undefined: Errno
/home/dfc/work/solaris-amd64-smartos-5f58c6d04b6d/go/src/pkg/syscall/zerrors_solaris_amd64.go:1077:
const initializer Errno(13) is not a constant
/home/dfc/work/solaris-amd64-smartos-5f58c6d04b6d/go/src/pkg/syscall/zerrors_solaris_amd64.go:1078:
undefined: Errno
/home/dfc/work/solaris-amd64-smartos-5f58c6d04b6d/go/src/pkg/syscall/zerrors_solaris_amd64.go:1078:
const initializer Errno(125) is not a constant
/home/dfc/work/solaris-amd64-smartos-5f58c6d04b6d/go/src/pkg/syscall/zerrors_solaris_amd64.go:1079:
undefined: Errno
/home/dfc/work/solaris-amd64-smartos-5f58c6d04b6d/go/src/pkg/syscall/zerrors_solaris_amd64.go:1079:
const initializer Errno(126) is not a constant
/home/dfc/work/solaris-amd64-smartos-5f58c6d04b6d/go/src/pkg/syscall/zerrors_solaris_amd64.go:1080:
undefined: Errno
/home/dfc/work/solaris-amd64-smartos-5f58c6d04b6d/go/src/pkg/syscall/zerrors_solaris_amd64.go:1080:
const initializer Errno(68) is not a constant
/home/dfc/work/solaris-amd64-smartos-5f58c6d04b6d/go/src/pkg/syscall/zerrors_solaris_amd64.go:1080:
too many errors



On Tue, Feb 25, 2014 at 7:12 PM, <mikioh.mikioh@gmail.com> wrote:

> *** Submitted as
> https://code.google.com/p/go/source/detail?r=5f58c6d04b6d ***
>
> syscall: generate zfiles for GOOS=solaris
>
> LGTM=minux.ma, mikioh.mikioh
> R=golang-codereviews, minux.ma, gobot, jsing, mikioh.mikioh, dave
> CC=golang-codereviews
> https://codereview.appspot.com/36010043
>
> Committer: Mikio Hara <mikioh.mikioh@gmail.com>
>
>
> https://codereview.appspot.com/36010043/
>

[gobot, 2014-02-25 08:26:42]

This CL appears to have broken the windows-386-ec2 builder.