Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1)

Issue 174960043: [dev.cc] code review 174960043: runtime: convert Solaris port to Go (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 3 months ago by aram
Modified:
10 years, 3 months ago
Reviewers:
rsc
CC:
rsc, dave_cheney.net, golang-codereviews, iant, khr, minux, r, rlh
Visibility:
Public.

Description

runtime: convert Solaris port to Go Memory management was consolitated with the BSD ports, since it was almost identical. Assembly thunks are gone, being replaced by the new //go:linkname feature. This change supersedes CL 138390043 (runtime: convert solaris netpoll to Go), which was previously reviewed and tested. This change is only the first step, the port now builds, but doesn't run. Binaries fail to exec: ld.so.1: 6.out: fatal: 6.out: TLS requirement failure : TLS support is unavailable Killed This seems to happen because binaries don't link with libc.so anymore. We will have to solve that in a different CL. Also this change is just a rough translation of the original C code, cleanup will come in a different CL. [This CL is part of the removal of C code from package runtime. See golang.org/s/dev.cc for an overview.]

Patch Set 1 #

Patch Set 2 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 3 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 4 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 5 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 6 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 7 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 8 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 9 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 10 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 11 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 12 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 13 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 14 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 15 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 16 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 17 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 18 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 19 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 20 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 ssh://hg@bitbucket.org/goarm64/go #

Patch Set 21 : diff -r bddf3876d57ec0c6267cfea84ae128f18a815e45 https://code.google.com/p/go #

Total comments: 14

Patch Set 22 : diff -r c6ca074d4454d059449d7d10599c8857a59a3eb0 https://code.google.com/p/go #

Patch Set 23 : diff -r c6ca074d4454d059449d7d10599c8857a59a3eb0 https://code.google.com/p/go #

Patch Set 24 : diff -r c6ca074d4454d059449d7d10599c8857a59a3eb0 https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+773 lines, -1090 lines) Patch
M src/liblink/asm6.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +2 lines, -3 lines 0 comments Download
M src/runtime/defs1_solaris_amd64.go View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +5 lines, -0 lines 0 comments Download
M src/runtime/mem_bsd.go View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
R src/runtime/mem_solaris.c View 1 2 3 1 chunk +0 lines, -101 lines 0 comments Download
M src/runtime/netpoll.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 6 chunks +12 lines, -38 lines 0 comments Download
M src/runtime/netpoll_solaris.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 8 chunks +121 lines, -142 lines 0 comments Download
M src/runtime/os2_solaris.go View 1 2 3 4 5 6 7 8 1 chunk +8 lines, -50 lines 0 comments Download
M src/runtime/os3_solaris.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +373 lines, -437 lines 0 comments Download
M src/runtime/os_solaris.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 5 chunks +17 lines, -35 lines 0 comments Download
M src/runtime/signal_solaris.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +81 lines, -90 lines 0 comments Download
M src/runtime/signal_solaris_amd64.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +39 lines, -24 lines 0 comments Download
M src/runtime/stubs.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 3 chunks +0 lines, -14 lines 0 comments Download
A src/runtime/stubs2.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +27 lines, -0 lines 0 comments Download
M src/runtime/sys_solaris_amd64.s View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 5 chunks +6 lines, -6 lines 0 comments Download
M src/runtime/syscall2_solaris.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +43 lines, -19 lines 0 comments Download
M src/runtime/syscall_solaris.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 20 chunks +38 lines, -41 lines 0 comments Download
R src/runtime/thunk_solaris_amd64.s View 1 2 3 4 5 1 chunk +0 lines, -89 lines 0 comments Download

Messages

Total messages: 8
aram
Hello rsc (cc: dfc, golang-codereviews@googlegroups.com, iant, khr, minux, r, rlh), I'd like you to review ...
10 years, 3 months ago (2014-11-12 15:10:32 UTC) #1
aram
> I'd like you to review this change to the dev.cc branch of > ssh://hg@bitbucket.org/goarm64/go ...
10 years, 3 months ago (2014-11-12 15:21:15 UTC) #2
aram
Hello rsc@golang.org (cc: dave@cheney.net, golang-codereviews@googlegroups.com, iant@golang.org, khr@golang.org, minux@golang.org, r@golang.org, rlh@golang.org), Please take another look.
10 years, 3 months ago (2014-11-12 15:21:50 UTC) #3
aram
On 2014/11/12 15:21:15, aram wrote: > > I'd like you to review this change to ...
10 years, 3 months ago (2014-11-12 15:22:26 UTC) #4
rsc
LGTM https://codereview.appspot.com/174960043/diff/370001/src/runtime/netpoll_solaris.go File src/runtime/netpoll_solaris.go (right): https://codereview.appspot.com/174960043/diff/370001/src/runtime/netpoll_solaris.go#newcode70 src/runtime/netpoll_solaris.go:70: //go:dynimport libc_port_create port_create "libc.so" this does nothing. in ...
10 years, 3 months ago (2014-11-12 18:55:34 UTC) #5
dave_cheney.net
This change looks good. Please address the points rsc raised and I'll test it on ...
10 years, 3 months ago (2014-11-13 06:58:03 UTC) #6
rsc
Please address the points I raised and just submit it. It's okay that it doesn't ...
10 years, 3 months ago (2014-11-13 15:03:46 UTC) #7
aram
10 years, 3 months ago (2014-11-13 15:07:35 UTC) #8
*** Submitted as https://code.google.com/p/go/source/detail?r=cc53451c5af3 ***

[dev.cc] runtime: convert Solaris port to Go

Memory management was consolitated with the BSD ports, since
it was almost identical.

Assembly thunks are gone, being replaced by the new //go:linkname
feature.

This change supersedes CL 138390043 (runtime: convert solaris
netpoll to Go), which was previously reviewed and tested.

This change is only the first step, the port now builds,
but doesn't run. Binaries fail to exec:

    ld.so.1: 6.out: fatal: 6.out: TLS requirement failure : TLS support is
unavailable
    Killed

This seems to happen because binaries don't link with libc.so
anymore. We will have to solve that in a different CL.

Also this change is just a rough translation of the original
C code, cleanup will come in a different CL.

[This CL is part of the removal of C code from package runtime.
See golang.org/s/dev.cc for an overview.]

LGTM=rsc
R=rsc, dave
CC=golang-codereviews, iant, khr, minux, r, rlh
https://codereview.appspot.com/174960043

https://codereview.appspot.com/174960043/diff/370001/src/runtime/netpoll_sola...
File src/runtime/netpoll_solaris.go (right):

https://codereview.appspot.com/174960043/diff/370001/src/runtime/netpoll_sola...
src/runtime/netpoll_solaris.go:70: //go:dynimport libc_port_create port_create
"libc.so"
On 2014/11/12 18:55:34, rsc wrote:
> this does nothing.
> in the C compiler, dynimport is a deprecated alias for cgo_import_dynamic.
> in the Go compiler, only cgo_import_dynamic is recgonized.

Done.

https://codereview.appspot.com/174960043/diff/370001/src/runtime/netpoll_sola...
src/runtime/netpoll_solaris.go:78: //go:linkname libc·port_getn libc·port_getn
On 2014/11/12 18:55:33, rsc wrote:
> this does nothing. you can't have a go variable named libc·port_getn. there is
> nothing for it to affect
> 
Good catch, "Edit ," failure, done.

https://codereview.appspot.com/174960043/diff/370001/src/runtime/os3_solaris.go
File src/runtime/os3_solaris.go (right):

https://codereview.appspot.com/174960043/diff/370001/src/runtime/os3_solaris....
src/runtime/os3_solaris.go:9: //go:dynexport end _end
On 2014/11/12 18:55:34, rsc wrote:
> cgo_export_dynamic
> 
> also, probably you mean
> //go:cgo_export_dynamic runtime.end _end
> 
> the first argument is a linkname, not a local variable name.
> unless you have changed the default linkname for the local variable end,
> it will be runtime.end.
> 
> (and it needs to be in package runtime for something else.)
> 
> same for the others

Done.

https://codereview.appspot.com/174960043/diff/370001/src/runtime/os3_solaris....
src/runtime/os3_solaris.go:13: //go:dynimport libc____errno ___errno "libc.so"
On 2014/11/12 18:55:34, rsc wrote:
> cgo_import_dynamic

Done.

https://codereview.appspot.com/174960043/diff/370001/src/runtime/os3_solaris....
src/runtime/os3_solaris.go:202: // m.procid is a uint64, but thr_new writes a
uint32 on 32-bit systems.
On 2014/11/12 18:55:34, rsc wrote:
> I don't see anything initializing procid in any way, so this is probably
> unnecessary. There's also not a thr_new.
> 

Done.

https://codereview.appspot.com/174960043/diff/370001/src/runtime/os_solaris.go
File src/runtime/os_solaris.go (right):

https://codereview.appspot.com/174960043/diff/370001/src/runtime/os_solaris.g...
src/runtime/os_solaris.go:9: func sysctl(mib *uint32, miblen uint32, out *byte,
size *uintptr, dst *byte, ndst uintptr) int32
On 2014/11/12 18:55:34, rsc wrote:
> this needs a //go:noescape if you ever call it. maybe you don't in which case
it
> can just go away.

Done (removed).

https://codereview.appspot.com/174960043/diff/370001/src/runtime/syscall2_sol...
File src/runtime/syscall2_solaris.go (right):

https://codereview.appspot.com/174960043/diff/370001/src/runtime/syscall2_sol...
src/runtime/syscall2_solaris.go:9: //go:dynimport libc_chdir chdir "libc.so"
On 2014/11/12 18:55:34, rsc wrote:
> cgo_import_dynamic

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b