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.