code review 35990043: runtime: add support for GOOS=solaris

runtime: add support for GOOS=solaris

[aram, 2013-12-02 20:44:45]

CC alex Some (trivial) windows changes in this one.

[brainman, 2013-12-04 05:51:48]

windows related code looks fine to me. But, please, ping me before submitting - I will test it on windows, if you like. Alex

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

Hello alex.brainman@gmail.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go

[brainman, 2013-12-10 22:25:59]

Build fails with: # ./all.bash # Building C bootstrap tool. cmd/dist # Building compilers and Go bootstrap tool for host, linux/386. lib9 libbio libmach liblink misc/pprof cmd/addr2line cmd/nm cmd/objdump cmd/pack cmd/prof cmd/cc cmd/gc cmd/8l cmd/8a cmd/8c cmd/8g pkg/runtime /root/go/root/src/pkg/runtime/mem_solaris.c:13 external redeclaration of: ENOMEM GLOBL ENUM /root/go/root/src/pkg/runtime/mem_solaris.c:13 GLOBL ENUM <unknown line number> go tool dist: FAILED: /root/go/root/pkg/tool/linux_386/8c -F -V -w -I $WORK -I /root/go/root/pkg/linux_386 -D GOOS_linux -D GOARCH_386 -D GOOS_GOARCH_linux_386 -o $WORK/mem_solaris.o /root/go/root/src/pkg/runtime/mem_solaris.c #

[dave_cheney.net, 2013-12-10 22:47:20]

Thanks for reviewing this, if you apply https://codereview.appspot.com/35900045/ First, do it fix the problem? > On 11 Dec 2013, at 9:25, alex.brainman@gmail.com wrote: > > Build fails with: > > # ./all.bash > > # Building C bootstrap tool. > > cmd/dist > > > > # Building compilers and Go bootstrap tool for host, linux/386. > > lib9 > > libbio > > libmach > > liblink > > misc/pprof > > cmd/addr2line > > cmd/nm > > cmd/objdump > > cmd/pack > > cmd/prof > > cmd/cc > > cmd/gc > > cmd/8l > > cmd/8a > > cmd/8c > > cmd/8g > > pkg/runtime > > /root/go/root/src/pkg/runtime/mem_solaris.c:13 external redeclaration > of: ENOMEM > GLOBL ENUM /root/go/root/src/pkg/runtime/mem_solaris.c:13 > > GLOBL ENUM <unknown line number> > > go tool dist: FAILED: /root/go/root/pkg/tool/linux_386/8c -F -V -w -I > $WORK -I /root/go/root/pkg/linux_386 -D GOOS_linux -D GOARCH_386 -D > GOOS_GOARCH_linux_386 -o $WORK/mem_solaris.o > /root/go/root/src/pkg/runtime/mem_solaris.c > # > > > > https://codereview.appspot.com/35990043/ > > -- > > ---You received this message because you are subscribed to the Google Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out. 

[brainman, 2013-12-10 23:09:35]

LGTM, but wait for others. All builds fine on both windows/386 and windows/amd64. Thank you. Alex

[brainman, 2013-12-10 23:10:06]

I forgotten to publish my comments. Alex https://codereview.appspot.com/35990043/diff/190001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/35990043/diff/190001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:213: // runtime·newproc1(&scavenger, nil, 0, 0, runtime·main); Why? Should it be part of this large CL? https://codereview.appspot.com/35990043/diff/190001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/35990043/diff/190001/src/pkg/runtime/runtime.h... src/pkg/runtime/runtime.h:351: LibCall	libcall;	// put here to avoid large runtime·stdcall stack This comment is bit confusing. Please, replace it with words you've used in GOOS_solaris part: // there are here because they are too large to be on the stack // of low-level NOSPLIT functions.

[aram, 2013-12-10 23:27:57]

Thanks, the CLs are not independent (as you have discovered). -- Aram Hăvărneanu 

[jsing, 2013-12-11 03:40:06]

Doing this via cgo/libc seems to be the wrong approach, given that we have the ability to do direct system calls. Discussing this with Aram, it seems that this was done to support both Solaris and Illumos/SmartOS. Do we know what additional overhead this creates? Would it be better to implement this as separate ports?

[aram, 2013-12-11 11:12:11]

Solaris is like windows. Direct system calls are not supported at all. In fact, it's worse than Windows. In Windows the syscalls are officially unsupported, but stable. In Solaris, they are unsupported, and also unstable. Really unstable. There is no other option but to use libc. Also the signal stuff depends on mutexes and other stuff inside libc. The signal trampoline is called by some libc function, with a large already setup, and havoc will happen if we don't use the pthread functions inside a signal handler. -- Aram Hăvărneanu 

[aram, 2013-12-11 14:04:57]

On Wed, Dec 11, 2013 at 4:40 AM, <jsing@google.com> wrote: > it seems that this was done to support both Solaris and Illumos/SmartOS. To clarify my previous mail. This wasn't done to support both Solaris and illumos (Solaris working is an artefact of the implementation, but it wasn't in the primary goals of this port); it was done because it's the only supported way to do it in Solaris-derived systems. -- Aram Hăvărneanu 

[aram, 2013-12-12 15:11:56]

PTAL https://codereview.appspot.com/35990043/diff/190001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/35990043/diff/190001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:213: // runtime·newproc1(&scavenger, nil, 0, 0, runtime·main); On 2013/12/10 23:10:07, brainman wrote: > Why? Should it be part of this large CL? Nope. Good catch. https://codereview.appspot.com/35990043/diff/190001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:213: // runtime·newproc1(&scavenger, nil, 0, 0, runtime·main); On 2013/12/10 23:10:07, brainman wrote: > Why? Should it be part of this large CL? Done. https://codereview.appspot.com/35990043/diff/190001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/35990043/diff/190001/src/pkg/runtime/runtime.h... src/pkg/runtime/runtime.h:351: LibCall	libcall;	// put here to avoid large runtime·stdcall stack On 2013/12/10 23:10:07, brainman wrote: > This comment is bit confusing. Please, replace it with words you've used in > GOOS_solaris part: > > // there are here because they are too large to be on the stack > // of low-level NOSPLIT functions. Done.

[gobot, 2013-12-20 05:20:09]

R=rsc@golang.org (assigned by alex.brainman@gmail.com)

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

Replacing golang-dev with golang-codereviews.

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

Hello alex.brainman@gmail.com, dave@cheney.net, jsing@google.com, gobot@golang.org, rsc@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.

[aram, 2013-12-29 17:44:41]

Hello alex.brainman@gmail.com, dave@cheney.net, jsing@google.com, gobot@golang.org, rsc@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.

[jsing, 2014-01-07 13:05:55]

Please update any files that have been copied from an existing source, so that they reflect the branch/revision history. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/asm_amd64.s File src/pkg/runtime/asm_amd64.s (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/asm_amd64... src/pkg/runtime/asm_amd64.s:621: MOVQ	m_gsignal(BP), SI It would be preferable to avoid using SI here, so that we do not have to reload m_g0 three instructions later. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/defs_sola... File src/pkg/runtime/defs_solaris.go (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/defs_sola... src/pkg/runtime/defs_solaris.go:1: // Copyright 2009 The Go Authors. All rights reserved. This was presumably copied from an existing defs_*.go - please update the change to reflect the source. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/defs_sola... File src/pkg/runtime/defs_solaris_amd64.go (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/defs_sola... src/pkg/runtime/defs_solaris_amd64.go:1: // Copyright 2013 The Go Authors. All rights reserved. Copied from defs_netbsd_amd64.go? https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/defs_sola... src/pkg/runtime/defs_solaris_amd64.go:45: REG_RFLAGS = C.REG_RFL // TODO(dfc) fixup Please expand the TODO and/or reference an issue. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/malloc.h File src/pkg/runtime/malloc.h (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/malloc.h#... src/pkg/runtime/malloc.h:505: void runtime_netpoll_scan(void (*)(Obj)); This looks like there are spaces between 'void' and the function name - please use a tab to make it consistent. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/mem_solar... File src/pkg/runtime/mem_solaris.c (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/mem_solar... src/pkg/runtime/mem_solaris.c:1: // Copyright 2013 The Go Authors. All rights reserved. Copied from another mem_*.c file? https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/netpoll.goc File src/pkg/runtime/netpoll.goc (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/netpoll.g... src/pkg/runtime/netpoll.goc:118: void runtime·netpollarmwrite(uintptr fd); The function prototypes should be moved to the top of the file (still in a #ifdef GOOS_solaris block). The rest of the netpoll prototypes are in runtime.h - it might even make sense to place them there. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/os_solaris.c File src/pkg/runtime/os_solaris.c (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/os_solari... src/pkg/runtime/os_solaris.c:1: // Copyright 2011 The Go Authors. All rights reserved. Copied from elsewhere? https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/os_solari... File src/pkg/runtime/os_solaris_amd64.c (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/os_solari... src/pkg/runtime/os_solaris_amd64.c:7: #include "os_GOOS.h" Is this file actually needed? https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/runtime.h... src/pkg/runtime/runtime.h:367: } scratch; Probably worth a comment to explain why these two structs exist here. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/stack.c File src/pkg/runtime/stack.c (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:260: // BUG(aram): at least check if it's on g0. s/BUG/TODO/

[aram, 2014-01-09 15:09:25]

https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/asm_amd64.s File src/pkg/runtime/asm_amd64.s (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/asm_amd64... src/pkg/runtime/asm_amd64.s:621: MOVQ	m_gsignal(BP), SI On 2014/01/07 13:05:56, jsing wrote: > It would be preferable to avoid using SI here, so that we do not have to reload > m_g0 three instructions later. Happy to do so, but what else to use? ISTM whatever register we use, we still have to do some save/restore dance. At least now it's explicit. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/defs_sola... File src/pkg/runtime/defs_solaris.go (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/defs_sola... src/pkg/runtime/defs_solaris.go:1: // Copyright 2009 The Go Authors. All rights reserved. On 2014/01/07 13:05:56, jsing wrote: > This was presumably copied from an existing defs_*.go - please update the change > to reflect the source. Done. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/defs_sola... File src/pkg/runtime/defs_solaris_amd64.go (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/defs_sola... src/pkg/runtime/defs_solaris_amd64.go:1: // Copyright 2013 The Go Authors. All rights reserved. On 2014/01/07 13:05:56, jsing wrote: > Copied from defs_netbsd_amd64.go? Done. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/defs_sola... src/pkg/runtime/defs_solaris_amd64.go:45: REG_RFLAGS = C.REG_RFL // TODO(dfc) fixup On 2014/01/07 13:05:56, jsing wrote: > Please expand the TODO and/or reference an issue. No idea what this is, removed. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/malloc.h File src/pkg/runtime/malloc.h (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/malloc.h#... src/pkg/runtime/malloc.h:505: void runtime_netpoll_scan(void (*)(Obj)); On 2014/01/07 13:05:56, jsing wrote: > This looks like there are spaces between 'void' and the function name - please > use a tab to make it consistent. Done. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/mem_solar... File src/pkg/runtime/mem_solaris.c (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/mem_solar... src/pkg/runtime/mem_solaris.c:1: // Copyright 2013 The Go Authors. All rights reserved. On 2014/01/07 13:05:56, jsing wrote: > Copied from another mem_*.c file? Done. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/netpoll.goc File src/pkg/runtime/netpoll.goc (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/netpoll.g... src/pkg/runtime/netpoll.goc:118: void runtime·netpollarmwrite(uintptr fd); On 2014/01/07 13:05:56, jsing wrote: > The function prototypes should be moved to the top of the file (still in a > #ifdef GOOS_solaris block). The rest of the netpoll prototypes are in runtime.h > - it might even make sense to place them there. Done. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/os_solaris.c File src/pkg/runtime/os_solaris.c (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/os_solari... src/pkg/runtime/os_solaris.c:1: // Copyright 2011 The Go Authors. All rights reserved. On 2014/01/07 13:05:56, jsing wrote: > Copied from elsewhere? Done. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/os_solari... File src/pkg/runtime/os_solaris_amd64.c (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/os_solari... src/pkg/runtime/os_solaris_amd64.c:7: #include "os_GOOS.h" On 2014/01/07 13:05:56, jsing wrote: > Is this file actually needed? No. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/os_solari... src/pkg/runtime/os_solaris_amd64.c:7: #include "os_GOOS.h" On 2014/01/07 13:05:56, jsing wrote: > Is this file actually needed? Done. https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/runtime.h... src/pkg/runtime/runtime.h:367: } scratch; On 2014/01/07 13:05:56, jsing wrote: > Probably worth a comment to explain why these two structs exist here. There is a comment above, is it not clear enough? https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/stack.c File src/pkg/runtime/stack.c (right): https://codereview.appspot.com/35990043/diff/330001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:260: // BUG(aram): at least check if it's on g0. On 2014/01/07 13:05:56, jsing wrote: > s/BUG/TODO/ Nah, this is a vestigial comment. Removed.

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

Hello alex.brainman@gmail.com, dave@cheney.net, jsing@google.com, gobot@golang.org, rsc@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.

[rsc, 2014-01-09 23:51:07]

LGTM Leaving for jsing (thanks!) https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/asm_amd64.s File src/pkg/runtime/asm_amd64.s (right): https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/asm_amd64... src/pkg/runtime/asm_amd64.s:620: JE	nosave please use JEQ; it is the more common spelling here https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/asm_amd64... src/pkg/runtime/asm_amd64.s:623: JE	nosave JEQ https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/cgocall.c File src/pkg/runtime/cgocall.c (right): https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/cgocall.c... src/pkg/runtime/cgocall.c:107: if(!runtime·iscgo && !Windows && !Sunos) this variable should be called Solaris, to match everything else https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/os_solaris.c File src/pkg/runtime/os_solaris.c (right): https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/os_solari... src/pkg/runtime/os_solaris.c:118: int32 n = (int32)runtime·sysconf(_SC_NPROCESSORS_ONLN); keep decl and init separate. and avoid ternary operator. int32 n; n = xxx if(n < 1) n = 1; return n https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/os_solari... src/pkg/runtime/os_solaris.c:339: if (runtime·sem_init(sem, 0, 0) != 0) global search and replace "if (" => "if(" https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/os_solari... src/pkg/runtime/os_solaris.c:359: if(*((int32*)(m->perrno)) != 0) { too many parens if(*(int32*)m->perrno == EINTR) https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/os_solari... src/pkg/runtime/os_solaris.c:375: if(*((int32*)(m->perrno)) == EINTR) again https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/runtime.h... src/pkg/runtime/runtime.h:358: void*	perrno;	// pointer to TLS errno why not make this an int32* and avoid the casts entirely?

[aram, 2014-01-11 14:10:48]

Hello alex.brainman@gmail.com, dave@cheney.net, jsing@google.com, gobot@golang.org, rsc@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.

[aram, 2014-01-11 14:10:51]

https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/asm_amd64.s File src/pkg/runtime/asm_amd64.s (right): https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/asm_amd64... src/pkg/runtime/asm_amd64.s:620: JE	nosave On 2014/01/09 23:51:08, rsc wrote: > please use JEQ; it is the more common spelling here Done. https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/asm_amd64... src/pkg/runtime/asm_amd64.s:623: JE	nosave On 2014/01/09 23:51:08, rsc wrote: > JEQ Done. https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/cgocall.c File src/pkg/runtime/cgocall.c (right): https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/cgocall.c... src/pkg/runtime/cgocall.c:107: if(!runtime·iscgo && !Windows && !Sunos) On 2014/01/09 23:51:08, rsc wrote: > this variable should be called Solaris, to match everything else Done. https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/os_solaris.c File src/pkg/runtime/os_solaris.c (right): https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/os_solari... src/pkg/runtime/os_solaris.c:118: int32 n = (int32)runtime·sysconf(_SC_NPROCESSORS_ONLN); On 2014/01/09 23:51:08, rsc wrote: > keep decl and init separate. and avoid ternary operator. > > int32 n; > > n = xxx > if(n < 1) > n = 1; > return n Done. https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/os_solari... src/pkg/runtime/os_solaris.c:339: if (runtime·sem_init(sem, 0, 0) != 0) On 2014/01/09 23:51:08, rsc wrote: > global search and replace "if (" => "if(" Done. https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/os_solari... src/pkg/runtime/os_solaris.c:359: if(*((int32*)(m->perrno)) != 0) { On 2014/01/09 23:51:08, rsc wrote: > too many parens > > if(*(int32*)m->perrno == EINTR) Done. https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/os_solari... src/pkg/runtime/os_solaris.c:375: if(*((int32*)(m->perrno)) == EINTR) On 2014/01/09 23:51:08, rsc wrote: > again Done. https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/35990043/diff/370001/src/pkg/runtime/runtime.h... src/pkg/runtime/runtime.h:358: void*	perrno;	// pointer to TLS errno On 2014/01/09 23:51:08, rsc wrote: > why not make this an int32* and avoid the casts entirely? Done.

[jsing, 2014-01-14 14:21:44]

LGTM https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/malloc.h File src/pkg/runtime/malloc.h (right): https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/malloc.h#... src/pkg/runtime/malloc.h:553: void	runtime_netpoll_scan(void (*)(Obj)); This is seemingly unused and should be reverted. https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/os_solaris.c File src/pkg/runtime/os_solaris.c (right): https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/os_solari... src/pkg/runtime/os_solaris.c:1: // Copyright 2011 The Go Authors. All rights reserved. This appears to be based on os_freebsd.c, however Rietveld is not showing it (missed branch history?). https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/signals_s... File src/pkg/runtime/signals_solaris.h (right): https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/signals_s... src/pkg/runtime/signals_solaris.h:55: /* BUG(aram): what should be do about these signals? D or N? is this set static? */ s/BUG/TODO/ https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/sys_solar... File src/pkg/runtime/sys_solaris_amd64.s (right): https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/sys_solar... src/pkg/runtime/sys_solaris_amd64.s:80: MOVL	$0,	0(DX) Single space between "," and "0" to match the rest of this file. https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/syscall_s... File src/pkg/runtime/syscall_solaris.goc (right): https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/syscall_s... src/pkg/runtime/syscall_solaris.goc:168: LibCall c; Most of the above are variable declarations, blank line, USED() and then the rest of the code. It would be nice if the rest followed the same convention.

[jsing, 2014-01-14 14:27:32]

LGTM

[aram, 2014-01-16 13:48:48]

Hello alex.brainman@gmail.com, dave@cheney.net, jsing@google.com, gobot@golang.org, rsc@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.

[aram, 2014-01-16 13:49:15]

https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/malloc.h File src/pkg/runtime/malloc.h (right): https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/malloc.h#... src/pkg/runtime/malloc.h:553: void	runtime_netpoll_scan(void (*)(Obj)); On 2014/01/14 14:21:45, jsing wrote: > This is seemingly unused and should be reverted. Done. https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/os_solaris.c File src/pkg/runtime/os_solaris.c (right): https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/os_solari... src/pkg/runtime/os_solaris.c:1: // Copyright 2011 The Go Authors. All rights reserved. On 2014/01/14 14:21:45, jsing wrote: > This appears to be based on os_freebsd.c, however Rietveld is not showing it > (missed branch history?). Done (again, I hate rietveld). https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/signals_s... File src/pkg/runtime/signals_solaris.h (right): https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/signals_s... src/pkg/runtime/signals_solaris.h:55: /* BUG(aram): what should be do about these signals? D or N? is this set static? */ On 2014/01/14 14:21:45, jsing wrote: > s/BUG/TODO/ Done. https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/sys_solar... File src/pkg/runtime/sys_solaris_amd64.s (right): https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/sys_solar... src/pkg/runtime/sys_solaris_amd64.s:80: MOVL	$0,	0(DX) On 2014/01/14 14:21:45, jsing wrote: > Single space between "," and "0" to match the rest of this file. Done. https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/syscall_s... File src/pkg/runtime/syscall_solaris.goc (right): https://codereview.appspot.com/35990043/diff/410001/src/pkg/runtime/syscall_s... src/pkg/runtime/syscall_solaris.goc:168: LibCall c; On 2014/01/14 14:21:45, jsing wrote: > Most of the above are variable declarations, blank line, USED() and then the > rest of the code. It would be nice if the rest followed the same convention. Done.

[jsing, 2014-01-17 04:59:31]

*** Submitted as https://code.google.com/p/go/source/detail?r=322f646feecb *** runtime: add support for GOOS=solaris R=alex.brainman, dave, jsing, gobot, rsc CC=golang-codereviews https://codereview.appspot.com/35990043 Committer: Joel Sing <jsing@google.com>

[dave_cheney.net, 2014-01-17 05:03:41]

\o/ Thanks! > On 17 Jan 2014, at 15:59, jsing@google.com wrote: > > *** Submitted as > https://code.google.com/p/go/source/detail?r=322f646feecb *** > > runtime: add support for GOOS=solaris > > R=alex.brainman, dave, jsing, gobot, rsc > CC=golang-codereviews > https://codereview.appspot.com/35990043 > > Committer: Joel Sing <jsing@google.com> > > > https://codereview.appspot.com/35990043/ 

[dvyukov, 2014-01-18 16:13:28]

https://codereview.appspot.com/35990043/diff/450001/src/pkg/runtime/lfstack.c File src/pkg/runtime/lfstack.c (right): https://codereview.appspot.com/35990043/diff/450001/src/pkg/runtime/lfstack.c... src/pkg/runtime/lfstack.c:23: // Use low-order three bits as ABA counter. 3 bits is kind of not quite enough for ABA counter the stack is currently used in GC, so we can require 12 low bits to be zero, but it still may be not enough, it's playing with fire

[aram, 2014-01-18 16:14:48]

https://codereview.appspot.com/35990043/diff/450001/src/pkg/runtime/lfstack.c File src/pkg/runtime/lfstack.c (right): https://codereview.appspot.com/35990043/diff/450001/src/pkg/runtime/lfstack.c... src/pkg/runtime/lfstack.c:23: // Use low-order three bits as ABA counter. On 2014/01/18 16:13:29, dvyukov wrote: > 3 bits is kind of not quite enough for ABA counter > > the stack is currently used in GC, so we can require 12 low bits to be zero, but > it still may be not enough, it's playing with fire This code is from gccgo.

[rsc, 2014-11-04 03:51:54]

https://codereview.appspot.com/35990043/diff/450001/src/pkg/runtime/lfstack.c File src/pkg/runtime/lfstack.c (right): https://codereview.appspot.com/35990043/diff/450001/src/pkg/runtime/lfstack.c... src/pkg/runtime/lfstack.c:23: // Use low-order three bits as ABA counter. How can it be that Solaris on AMD64 uses all 64 bits of virtual address? The chips don't allow that.

[minux, 2014-11-04 05:08:12]

https://codereview.appspot.com/35990043/diff/450001/src/pkg/runtime/lfstack.c File src/pkg/runtime/lfstack.c (right): https://codereview.appspot.com/35990043/diff/450001/src/pkg/runtime/lfstack.c... src/pkg/runtime/lfstack.c:23: // Use low-order three bits as ABA counter. On 2014/11/04 03:51:54, rsc wrote: > How can it be that Solaris on AMD64 uses all 64 bits of virtual address? The > chips don't allow that. I think that refers to the fact the Solaris kernel allocate part of the higher half of memory to user space, so you can't assume that highest bit of a user- space address is 0. This "feature" has broken quite some softwares that make assumptions on addresses (e.g. those that uses high-order bits to store tags in a pointer.)

[aram, 2014-11-04 09:55:04]

> I think that refers to the fact the Solaris kernel allocate part > of the higher half of memory to user space, so you can't assume > that highest bit of a user-space address is 0. That's correct. For example, the user-space stack starts at 0xfffffd8000000000. Note that bits<63-1:N> must be always a copy of bit<63>, with N currently 48.

[rsc, 2014-11-04 14:34:58]

Thanks, that's easy to fix. 

[gusev.vitaliy, 2018-03-01 17:23:13]

Approach to check "errno" after syscall works so far, but for forkx() it can fail by some conditions. What if use standard approach to check syscall return value on -1. https://codereview.appspot.com/35990043/diff/450001/src/pkg/runtime/sys_solar... File src/pkg/runtime/sys_solaris_amd64.s (right): https://codereview.appspot.com/35990043/diff/450001/src/pkg/runtime/sys_solar... src/pkg/runtime/sys_solaris_amd64.s:108: MOVQ	AX, libcall_err(DI) Why here is used errno as indicator of error from system call ? Why don't use AX value from line 99 and check it on "-1" ? POSIX and man pages says that errno value makes sense only when sys call return error, but not vice versa. Of course I see errno resetting on the line 80.

[iant, 2018-03-01 22:10:49]

On 2018/03/01 17:23:13, gusev.vitaliy wrote: You are replying to a patch that is three years old and has been committed. We also no longer use this code review system. If you think that current Go should change, I encourage you to open an issue at https://golang.org/issue or start a discussion on the golang-dev Google group. Thanks.