code review 68490043: all: merge NaCl branch (part 1)

all: merge NaCl branch (part 1) See golang.org/s/go13nacl for design overview. This CL is the mostly mechanical changes from rsc's Go 1.2 based NaCl branch, specifically 39cb35750369 to 500771b477cf from https://code.google.com/r/rsc-go13nacl. This CL does not include working NaCl support, there are probably two or three more large merges to come. CL 15750044 is not included as it involves more invasive changes to the linker which will need to be merged separately. The exact change lists included are 15050047: syscall: support for Native Client 15360044: syscall: unzip implementation for Native Client 15370044: syscall: Native Client SRPC implementation 15400047: cmd/dist, cmd/go, go/build, test: support for Native Client 15410048: runtime: support for Native Client 15410049: syscall: file descriptor table for Native Client 15410050: syscall: in-memory file system for Native Client 15440048: all: update +build lines for Native Client port 15540045: cmd/6g, cmd/8g, cmd/gc: support for Native Client 15570045: os: support for Native Client 15680044: crypto/..., hash/crc32, reflect, sync/atomic: support for amd64p32 15690044: net: support for Native Client 15690048: runtime: support for fake time like on Go Playground 15690051: build: disable various tests on Native Client

[dave_cheney.net, 2014-02-25 07:41:16]

Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://bitbucket.org/davecheney/go.nacl

[dave_cheney.net, 2014-02-25 07:43:17]

Sorry for the truly gargantuan CL. It clocks in at roughly 7.6k lines. rsc asked me to mail my progress merging his https://code.google.com/r/rsc-go13nacl branch into trunk. The changes to this point are mostly mechanical and have no impact on the current platforms. I can break this down into smaller units if required.

[lucio, 2014-02-25 07:57:53]

On 2/25/14, dave@cheney.net <dave@cheney.net> wrote: > ... > I can break this down into smaller units if required. > I vote against breaking it down, it looks like a phenomenal amount of additional effort :-) Lucio. 

[iant, 2014-02-25 17:59:33]

I read through this CL and made a few comments. Would you mind cleaning this up as appropriate? Thanks. https://codereview.appspot.com/68490043/diff/60001/src/pkg/net/file_unix.go File src/pkg/net/file_unix.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/net/file_unix.go#n... src/pkg/net/file_unix.go:27: println("getsockopt failed", fd, err.Error()) Adding this println looks wrong. https://codereview.appspot.com/68490043/diff/60001/src/pkg/net/port_unix.go File src/pkg/net/port_unix.go (left): https://codereview.appspot.com/68490043/diff/60001/src/pkg/net/port_unix.go#o... src/pkg/net/port_unix.go:15: // (some Solaris distros). This reverts a change from revision 19231:4ab152b54171 (https://codereview.appspot.com/36030043). https://codereview.appspot.com/68490043/diff/60001/src/pkg/net/sockopt_solari... File src/pkg/net/sockopt_solaris.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/net/sockopt_solari... src/pkg/net/sockopt_solaris.go:31: return os.NewSyscallError("setsockopt", syscall.SetsockoptInt(s, syscall.SOL_SOCKET, syscall.SO_REUSEPORT, 1)) This change seems unrelated. On the other hand, I'm not sure it was committed. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/arch_amd64... File src/pkg/runtime/arch_amd64p32.h (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/arch_amd64... src/pkg/runtime/arch_amd64p32.h:9: #ifdef GOOS_solaris I'm not sure it's necessary to test for GOOS_solaris in a NaCL specific file. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/arch_amd64... src/pkg/runtime/arch_amd64p32.h:14: #ifdef GOOS_nacl I'm also not sure it's necessary to test for GOOS_nacl, but perhaps it is for some reason. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (left): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/runtime.h#... src/pkg/runtime/runtime.h:777: * Goc2c also takes care of conveying to the garbage collector Why remove this part of the comment? https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/runtime.h#... src/pkg/runtime/runtime.h:34: //typedef	uint64	uintreg; Presumably there is no need for these commented out typedefs. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/runtime.h#... src/pkg/runtime/runtime.h:948: G*	runtime·timejump(void); This declaration should not be added. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/vlrt_arm.c File src/pkg/runtime/vlrt_arm.c (left): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/vlrt_arm.c... src/pkg/runtime/vlrt_arm.c:40: runtime·panicdivide(void) I don't entirely understand why this is going away.

[elias.naur, 2014-02-25 18:46:22]

https://codereview.appspot.com/68490043/diff/60001/src/pkg/crypto/sha1/sha1bl... File src/pkg/crypto/sha1/sha1block_decl.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/crypto/sha1/sha1bl... src/pkg/crypto/sha1/sha1block_decl.go:5: // +build amd64 amd64p32 386 This reverts a previous CL that added an assembly optimized version of block. You'll need to add "arm" to the tags again.

[bradfitz, 2014-02-25 18:54:48]

Fixing. On Tue, Feb 25, 2014 at 10:46 AM, <elias.naur@gmail.com> wrote: > > https://codereview.appspot.com/68490043/diff/60001/src/ > pkg/crypto/sha1/sha1block_decl.go > File src/pkg/crypto/sha1/sha1block_decl.go (right): > > https://codereview.appspot.com/68490043/diff/60001/src/ > pkg/crypto/sha1/sha1block_decl.go#newcode5 > src/pkg/crypto/sha1/sha1block_decl.go:5: // +build amd64 amd64p32 386 > This reverts a previous CL that added an assembly optimized version of > block. You'll need to add "arm" to the tags again. > > > https://codereview.appspot.com/68490043/ > > -- > 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. > 

[rsc, 2014-02-25 20:37:23]

R=close Dave sent me a different copy of this CL (as 68150047) and I missed that this one existed too. I submitted the other one.

[aram, 2014-02-25 22:44:53]

Thank you for doing this. https://codereview.appspot.com/68490043/diff/60001/src/pkg/go/build/deps_test.go File src/pkg/go/build/deps_test.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/go/build/deps_test... src/pkg/go/build/deps_test.go:396: if pkg == "runtime/cgo" && !ctxt.CgoEnabled { This seems redundant, does it not? https://codereview.appspot.com/68490043/diff/60001/src/pkg/hash/crc32/crc32_a... File src/pkg/hash/crc32/crc32_amd64x.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/hash/crc32/crc32_a... src/pkg/hash/crc32/crc32_amd64x.go:1: // Copyright 2011 The Go Authors. All rights reserved. Forgot hg cp. https://codereview.appspot.com/68490043/diff/60001/src/pkg/net/fd_poll_nacl.go File src/pkg/net/fd_poll_nacl.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/net/fd_poll_nacl.g... src/pkg/net/fd_poll_nacl.go:1: // Copyright 2013 The Go Authors. All rights reserved. 2014. https://codereview.appspot.com/68490043/diff/60001/src/pkg/os/stat_nacl.go File src/pkg/os/stat_nacl.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/os/stat_nacl.go#ne... src/pkg/os/stat_nacl.go:1: // Copyright 2009 The Go Authors. All rights reserved. This seems like a copy. If so, hg cp, otherwise 2014. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/arch_amd64.h File src/pkg/runtime/arch_amd64.h (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/arch_amd64... src/pkg/runtime/arch_amd64.h:14: #ifdef GOOS_nacl What's the point of this, doesn't nacl use arch_amd64p32.h? https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/mem_linux.c File src/pkg/runtime/mem_linux.c (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/mem_linux.... src/pkg/runtime/mem_linux.c:14: EACCES = 13, Why not in defs_*.h? https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/mem_nacl.c File src/pkg/runtime/mem_nacl.c (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/mem_nacl.c... src/pkg/runtime/mem_nacl.c:1: // Copyright 2010 The Go Authors. All rights reserved. hg cp. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/mem_nacl.c... src/pkg/runtime/mem_nacl.c:64: if(NaCl || sizeof(void*) == 8) NaCl is true right? Code that follows is dead code? https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/mknacl.sh File src/pkg/runtime/mknacl.sh (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/mknacl.sh#... src/pkg/runtime/mknacl.sh:2: # Copyright 2013 The Go Authors. All rights reserved. 2014. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/netpoll_na... File src/pkg/runtime/netpoll_nacl.c (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/netpoll_na... src/pkg/runtime/netpoll_nacl.c:1: // Copyright 2013 The Go Authors. All rights reserved. Why not use netpoll_stub.c? Also 2014. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/os_nacl.c File src/pkg/runtime/os_nacl.c (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/os_nacl.c#... src/pkg/runtime/os_nacl.c:1: // Copyright 2010 The Go Authors. All rights reserved. hg cp. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/os_nacl.c#... src/pkg/runtime/os_nacl.c:58: //runtime·nacl_exception_handler(runtime·sigtramp, nil); delete. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/os_nacl.c#... src/pkg/runtime/os_nacl.c:64: *(int32*)0 = 0; What happens when this is called from a signal handler in nacl? Don't we have do disable signals first? https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/os_nacl.c#... src/pkg/runtime/os_nacl.c:68: runtime·get_random_data(byte **rnd, int32 *rnd_len) TODO or explain why not needed. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/os_nacl.c#... src/pkg/runtime/os_nacl.c:223: void	runtime·madvise(byte*, uintptr, int32) { } Whitespace. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/os_nacl.c#... src/pkg/runtime/os_nacl.c:257: uint32 runtime·writelock; // test-and-set spin lock for runtime.write Why do we need this? https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/signals_na... File src/pkg/runtime/signals_nacl.h (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/signals_na... src/pkg/runtime/signals_nacl.h:1: // Copyright 2009 The Go Authors. All rights reserved. hg cp. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/syscall_na... File src/pkg/runtime/syscall_nacl.h (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/syscall_na... src/pkg/runtime/syscall_nacl.h:1: // generated by mknacl.sh - do not edit A new operating system and still has more system calls than Plan 9 :(. https://codereview.appspot.com/68490043/diff/60001/src/pkg/sync/atomic/asm_am... File src/pkg/sync/atomic/asm_amd64p32.s (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/sync/atomic/asm_am... src/pkg/sync/atomic/asm_amd64p32.s:1: // Copyright 2011 The Go Authors. All rights reserved. hg cp or 2014. https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/asm_nacl_3... File src/pkg/syscall/asm_nacl_386.s (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/asm_nacl_3... src/pkg/syscall/asm_nacl_386.s:1: // Copyright 2013 The Go Authors. All rights reserved. 2014. https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/asm_nacl_a... File src/pkg/syscall/asm_nacl_amd64p32.s (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/asm_nacl_a... src/pkg/syscall/asm_nacl_amd64p32.s:1: // Copyright 2013 The Go Authors. All rights reserved. 2014. https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/fd_nacl.go File src/pkg/syscall/fd_nacl.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/fd_nacl.go... src/pkg/syscall/fd_nacl.go:1: // Copyright 2013 The Go Authors. All rights reserved. hg cp or 2014. https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/fs_nacl.go File src/pkg/syscall/fs_nacl.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/fs_nacl.go... src/pkg/syscall/fs_nacl.go:1: // Copyright 2013 The Go Authors. All rights reserved. 2014. https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/srpc_nacl.go File src/pkg/syscall/srpc_nacl.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/srpc_nacl.... src/pkg/syscall/srpc_nacl.go:1: // Copyright 2013 The Go Authors. All rights reserved. 2014. https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/syscall_na... File src/pkg/syscall/syscall_nacl.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/syscall_na... src/pkg/syscall/syscall_nacl.go:1: // Copyright 2013 The Go Authors. All rights reserved. 2014. This code is such a joy. https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/syscall_na... File src/pkg/syscall/syscall_nacl_386.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/syscall_na... src/pkg/syscall/syscall_nacl_386.go:1: // Copyright 2013 The Go Authors. All rights reserved. 2014. https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/syscall_na... File src/pkg/syscall/syscall_nacl_amd64p32.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/syscall_na... src/pkg/syscall/syscall_nacl_amd64p32.go:1: // Copyright 2013 The Go Authors. All rights reserved. 2014. Isn't this identical with syscall_nacl_386.go? https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/time_nacl_... File src/pkg/syscall/time_nacl_386.s (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/syscall/time_nacl_... src/pkg/syscall/time_nacl_386.s:1: // Copyright 2013 The Go Authors. All rights reserved. 2014.

[rsc, 2014-02-26 15:19:32]

I'll take care of the cleanup below. Remember that this CL is already submitted under a different number (see earlier mail from me on this thread). https://codereview.appspot.com/68490043/diff/60001/src/pkg/go/build/deps_test.go File src/pkg/go/build/deps_test.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/go/build/deps_test... src/pkg/go/build/deps_test.go:396: if pkg == "runtime/cgo" && !ctxt.CgoEnabled { On 2014/02/25 22:44:54, aram wrote: > This seems redundant, does it not? No, it doesn't. Redundant with what? https://codereview.appspot.com/68490043/diff/60001/src/pkg/hash/crc32/crc32_a... File src/pkg/hash/crc32/crc32_amd64x.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/hash/crc32/crc32_a... src/pkg/hash/crc32/crc32_amd64x.go:1: // Copyright 2011 The Go Authors. All rights reserved. On 2014/02/25 22:44:54, aram wrote: > Forgot hg cp. Too late now. https://codereview.appspot.com/68490043/diff/60001/src/pkg/net/fd_poll_nacl.go File src/pkg/net/fd_poll_nacl.go (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/net/fd_poll_nacl.g... src/pkg/net/fd_poll_nacl.go:1: // Copyright 2013 The Go Authors. All rights reserved. On 2014/02/25 22:44:54, aram wrote: > 2014. These files are being copied from another repo and were checked in in 2013. As a matter of project policy, we do not both with the busy work of changing copyright years in source files. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/arch_amd64.h File src/pkg/runtime/arch_amd64.h (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/arch_amd64... src/pkg/runtime/arch_amd64.h:14: #ifdef GOOS_nacl On 2014/02/25 22:44:54, aram wrote: > What's the point of this, doesn't nacl use arch_amd64p32.h? Yes, thanks. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/arch_amd64... File src/pkg/runtime/arch_amd64p32.h (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/arch_amd64... src/pkg/runtime/arch_amd64p32.h:9: #ifdef GOOS_solaris On 2014/02/25 17:59:34, iant wrote: > I'm not sure it's necessary to test for GOOS_solaris in a NaCL specific file. Done. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/arch_amd64... src/pkg/runtime/arch_amd64p32.h:14: #ifdef GOOS_nacl On 2014/02/25 17:59:34, iant wrote: > I'm also not sure it's necessary to test for GOOS_nacl, but perhaps it is for > some reason. Maybe some day someone will want to run Linux in amd64p32 mode. It's only nacl with the wacky page size. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/mem_linux.c File src/pkg/runtime/mem_linux.c (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/mem_linux.... src/pkg/runtime/mem_linux.c:14: EACCES = 13, On 2014/02/25 22:44:54, aram wrote: > Why not in defs_*.h? I don't remember. It's not a big deal. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (left): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/runtime.h#... src/pkg/runtime/runtime.h:777: * Goc2c also takes care of conveying to the garbage collector On 2014/02/25 17:59:34, iant wrote: > Why remove this part of the comment? Bad merge. Fixed. https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/runtime.h#... src/pkg/runtime/runtime.h:34: //typedef	uint64	uintreg; On 2014/02/25 17:59:34, iant wrote: > Presumably there is no need for these commented out typedefs. There will be in the next CL (uncommented). https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/vlrt_arm.c File src/pkg/runtime/vlrt_arm.c (left): https://codereview.appspot.com/68490043/diff/60001/src/pkg/runtime/vlrt_arm.c... src/pkg/runtime/vlrt_arm.c:40: runtime·panicdivide(void) On 2014/02/25 17:59:34, iant wrote: > I don't entirely understand why this is going away. It was meant to move into portable code, because it is now needed on x86 too. But the CL missed the addition in the portable code part.

[rsc, 2014-02-26 15:20:44]

The fix CL is https://codereview.appspot.com/69200043.