code review 6222044: runtime: make go work on netbsd/amd64

runtime: make go work on netbsd/amd64

[jsing, 2012-05-18 16:23:12]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2012-05-22 03:03:48]

http://codereview.appspot.com/6222044/diff/5001/src/pkg/runtime/sys_netbsd_am...
File src/pkg/runtime/sys_netbsd_amd64.s (right):

http://codereview.appspot.com/6222044/diff/5001/src/pkg/runtime/sys_netbsd_am...
src/pkg/runtime/sys_netbsd_amd64.s:12: TEXT runtime·lwp_mcontext_init(SB),7,$0
Sure looks like this could be written in C instead.

http://codereview.appspot.com/6222044/diff/5001/src/pkg/runtime/sys_netbsd_am...
src/pkg/runtime/sys_netbsd_amd64.s:31: // Set up lwp_create args.
I appreciate the comments, but this is standard stuff for a system call: it's
okay to make this look like all the others:

TEXT 
    MOVQ
    MOVQ
    MOVQ
    MOVL
    SYSCALL
    JCC
    NEGL
    RET

http://codereview.appspot.com/6222044/diff/5001/src/pkg/runtime/thread_netbsd.c
File src/pkg/runtime/thread_netbsd.c (right):

http://codereview.appspot.com/6222044/diff/5001/src/pkg/runtime/thread_netbsd...
src/pkg/runtime/thread_netbsd.c:77: // TODO(jsing) - potential deadlock!
Please expand this comment to explain.

http://codereview.appspot.com/6222044/diff/5001/src/pkg/runtime/thread_netbsd...
src/pkg/runtime/thread_netbsd.c:84: // TODO(jsing) - potential deadlock!
Same.

[jsing, 2012-05-22 15:56:41]

PTAL

http://codereview.appspot.com/6222044/diff/5001/src/pkg/runtime/sys_netbsd_am...
File src/pkg/runtime/sys_netbsd_amd64.s (right):

http://codereview.appspot.com/6222044/diff/5001/src/pkg/runtime/sys_netbsd_am...
src/pkg/runtime/sys_netbsd_amd64.s:12: TEXT runtime·lwp_mcontext_init(SB),7,$0
On 2012/05/22 03:03:48, rsc wrote:
> Sure looks like this could be written in C instead.

Yes, it could be. However, since the code is MD we would need a
thread_netbsd_amd64.c and a thread_netbsd_386.c (or similar) - this seemed to be
overkill for a single function which could easily be written in assembly.

http://codereview.appspot.com/6222044/diff/5001/src/pkg/runtime/sys_netbsd_am...
src/pkg/runtime/sys_netbsd_amd64.s:31: // Set up lwp_create args.
On 2012/05/22 03:03:48, rsc wrote:
> I appreciate the comments, but this is standard stuff for a system call: it's
> okay to make this look like all the others:

Heh... yes, that is overly verbose - looks like I rewrote the existing comment
:)

http://codereview.appspot.com/6222044/diff/5001/src/pkg/runtime/thread_netbsd.c
File src/pkg/runtime/thread_netbsd.c (right):

http://codereview.appspot.com/6222044/diff/5001/src/pkg/runtime/thread_netbsd...
src/pkg/runtime/thread_netbsd.c:77: // TODO(jsing) - potential deadlock!
On 2012/05/22 03:03:48, rsc wrote:
> Please expand this comment to explain.

Done.

http://codereview.appspot.com/6222044/diff/5001/src/pkg/runtime/thread_netbsd...
src/pkg/runtime/thread_netbsd.c:84: // TODO(jsing) - potential deadlock!
On 2012/05/22 03:03:48, rsc wrote:
> Same.

Refers to the detailed description that is now included above.

[rsc, 2012-05-22 17:00:31]

On Tue, May 22, 2012 at 11:56 AM,  <jsing@google.com> wrote:
> Yes, it could be. However, since the code is MD we would need a
> thread_netbsd_amd64.c and a thread_netbsd_386.c (or similar) - this
> seemed to be overkill for a single function which could easily be
> written in assembly.

I argue that the overkill is writing functions in assembly that can be
written in C.
Use signal_netbsd_386.c and signal_netbsd_amd64.c, which already exist.

Russ

[jsing, 2012-05-22 17:21:43]

On 23 May 2012 03:00, Russ Cox <rsc@golang.org> wrote:

> On Tue, May 22, 2012 at 11:56 AM,  <jsing@google.com> wrote:
> > Yes, it could be. However, since the code is MD we would need a
> > thread_netbsd_amd64.c and a thread_netbsd_386.c (or similar) - this
> > seemed to be overkill for a single function which could easily be
> > written in assembly.
>
> I argue that the overkill is writing functions in assembly that can be
> written in C.
> Use signal_netbsd_386.c and signal_netbsd_amd64.c, which already exist.
>

Sure, I will do this, however it seems confusing since it is thread related
code and not signal related code.

[rsc, 2012-05-22 17:23:19]

On Tue, May 22, 2012 at 1:21 PM, Joel Sing <jsing@google.com> wrote:
> Sure, I will do this, however it seems confusing since it is thread related
> code and not signal related code.

Some day I might rename the files.  There's lots of non-signal stuff.

[jsing, 2012-05-23 11:51:38]

On 2012/05/22 17:23:19, rsc wrote:
> On Tue, May 22, 2012 at 1:21 PM, Joel Sing <mailto:jsing@google.com> wrote:
> > Sure, I will do this, however it seems confusing since it is thread related
> > code and not signal related code.
> 
> Some day I might rename the files.  There's lots of non-signal stuff.

Sounds good - sys_<GOOS>_<GOARCH>.c would make more sense and matches up with
the assembly files.

PTAL.

[rsc, 2012-05-23 17:50:23]

LGTM

[dho, 2012-05-23 17:54:58]

LGTM

http://codereview.appspot.com/6222044/diff/7002/src/pkg/runtime/thread_netbsd.c
File src/pkg/runtime/thread_netbsd.c (right):

http://codereview.appspot.com/6222044/diff/7002/src/pkg/runtime/thread_netbsd...
src/pkg/runtime/thread_netbsd.c:92: }else {
} else {

[jsing, 2012-05-24 01:33:25]

*** Submitted as http://code.google.com/p/go/source/detail?r=0baf69388fa5 ***

runtime: make go work on netbsd/amd64

R=golang-dev, rsc, devon.odell
CC=golang-dev
http://codereview.appspot.com/6222044