Re: Hurd port for gcc go PATCH 1-4(23)

[Samuel Thibault]

Hello,

Nice work!

Mmm, why making changes in each file in a separate patch?  I don't think
it makes things easier to review, I'd say rather send a big patch, it's
altogether that it makes sense anyway.

> Index: gcc-6-6.2.1-4.1/src/libgo/Makefile.in
> ===================================================================
> --- gcc-6-6.2.1-4.1.orig/src/libgo/Makefile.in
> +++ gcc-6-6.2.1-4.1/src/libgo/Makefile.in

Makefile.in is a generated file!  Send the patch against Makefile.am
instead.  I hope you didn't modify Makefile.in by hand?!


Svante Signell, on Fri 25 Nov 2016 20:59:27 +0100, wrote:
> Index: gcc-6-6.2.1-4.1/src/libgo/go/syscall/wait.c
> ===================================================================
> --- gcc-6-6.2.1-4.1.orig/src/libgo/go/syscall/wait.c
> +++ gcc-6-6.2.1-4.1/src/libgo/go/syscall/wait.c
> @@ -8,6 +8,9 @@
>     OS-independent.  */
>  
>  #include <stdint.h>
> +#ifndef WCONTINUED
> +#define WCONTINUED 0
> +#endif
>  #include <sys/wait.h>
>  
>  #include "runtime.h"

That looks odd at best: WCONTINUED can't be defined at this place
anyway, so it'll get defined to 0. Fortunately the sys/wait.h definition
would override this. But then wait.c will define something which does
not make sense since WCONTINUED is not implemented. Better use #ifdef
WCONTINUED inside the Continued function, to always return 0 when it's
not defined.


> @@ -744,6 +752,14 @@ go_net_sockoptip_file = go/net/sockoptip
>  go_net_cgo_sock_file = go/net/cgo_sockold.go
>  go_net_cgo_res_file = go/net/cgo_resnew.go
>  else
> +if LIBGO_IS_GNU
> +go_net_cgo_file = go/net/cgo_linux.go

I'd say rather rename to cgo_glibc.go . The getaddrinfo implementation
is simply exactly the same between linux, GNU/Hurd, kFreeBSD etc., there
is nothing Linux-specific here.

There are quite a few other files which could be renamed to glibc:

os/pipe_linux.go
crypto/x509/root_linux.go
syscall/errstr_linux.go


> @@ -768,11 +785,15 @@ else
>  if LIBGO_IS_SOLARIS
>  go_net_sendfile_file = go/net/sendfile_solaris.go
>  else
> +if LIBGO_IS_GNU
> +go_net_sendfile_file = go/net/sendfile_gnu.go
> +else
>  go_net_sendfile_file = go/net/sendfile_stub.go
>  endif

There is no difference with the Linux version... I'd say rather propose
to rename sendfile_linux.go into sendfile_glibc.go and just use that.


> +go_net_sock_file = go/net/sock_gnu.go

> Index: gcc-6-6.2.1-4.1/src/libgo/go/net/sock_gnu.go
> ===================================================================
> --- /dev/null
> +++ gcc-6-6.2.1-4.1/src/libgo/go/net/sock_gnu.go
> @@ -0,0 +1,14 @@
> +// Copyright 2014 The Go Authors.  All rights reserved.
> +// Use of this source code is governed by a BSD-style
> +// license that can be found in the LICENSE file.
> +
> +// +build gnu
> +
> +package net
> +
> +import "syscall"
> +
> +func maxListenerBacklog() int {
> +       // From /usr/include/i386-gnu/bits/socket.h
> +       return syscall.SOMAXCONN
> +}

Why not just using sock_stub.go which is the same?


> @@ -2028,11 +2093,20 @@ else
>  syscall_os_file = go/syscall/libcall_bsd.go
>  endif
>  
> +# GNU/Hurd specific library calls support.
> +if LIBGO_IS_GNU
> +syscall_libcall_file = go/syscall/libcall_posix-1.go
> +syscall_os_test_file = go/syscall/syscall_gnu_test.go
> +else
> +syscall_libcall_file = go/syscall/libcall_posix.go
> +syscall_os_test_file = go/syscall/syscall_unix_test.go
> +endif

That will be probably be very much frowned upon...

What you said is:
- Removed the mount call for GNU/Hurd, it exists but use translators.
- Removed the mlockall/munlockall calls for GNU/Hurd, not yet implemented.
- Removed the madvise call for GNU/Hurd, not yet implemented.

I'd say you'd much better see with upstream how to make these
conditional inside the file itself, using ifdef GNU for mount,
HAVE_MLOCKALL and such for mlockall, munlockall, madvise, rather than
forking it all...

And
src_libgo_go_syscall_syscall_gnu_test.go: New file:
  Define Type and Whence as 32bit in syscall.Flock_t

Again, you'll probably have to discuss with upstream to see how they
prefer to make it configurable rather than forking the whole file.


> @@ -4431,7 +4505,7 @@ mostlyclean-local:
>       find . -name '*-testsum' -print | xargs rm -f
>       find . -name '*-testlog' -print | xargs rm -f
>  
> -CLEANFILES = *.go *.gox goc2c *.c s-version libgo.sum libgo.log
> +CLEANFILES = *.go *.gox goc2c *.c s-* libgo.sum libgo.log

This seems unrelated?


> Index: gcc-6-6.2.1-4.1/src/libgo/mksysinfo.sh
> ===================================================================
> --- gcc-6-6.2.1-4.1.orig/src/libgo/mksysinfo.sh
> +++ gcc-6-6.2.1-4.1/src/libgo/mksysinfo.sh
> @@ -304,6 +304,13 @@ echo '#include <errno.h>' | ${CC} -x c -
>    egrep '#define E[A-Z0-9_]+ ' | \
>    sed -e 's/^#define \(E[A-Z0-9_]*\) .*$/const \1 = Errno(_\1)/' >> ${OUT}
>  
> +# Special treatment of EWOULDBLOCK for GNU/Hurd
> +# /usr/include/bits/errno.h: #define EWOULDBLOCK EAGAIN
> +if egrep 'define EWOULDBLOCK EAGAIN' gen-sysinfo.go > /dev/null 2>&1; then
> +  egrep '^const EWOULDBLOCK = Errno(_EWOULDBLOCK)' ${OUT} | \
> +    sed -i -e 's/_EWOULDBLOCK/_EAGAIN/' ${OUT}
> +fi
> +

Err, why using the second egrep at all? sed -i will work directly on the
file, it won't care about its input. Doesn't this just work already?

# Special treatment of EWOULDBLOCK for GNU/Hurd
# /usr/include/bits/errno.h: #define EWOULDBLOCK EAGAIN
if egrep 'define EWOULDBLOCK EAGAIN' gen-sysinfo.go > /dev/null 2>&1; then
    sed -i -e 's/_EWOULDBLOCK/_EAGAIN/' ${OUT}
fi


Svante Signell, on Fri 25 Nov 2016 21:04:58 +0100, wrote:
> * src_libgo_runtime_netpoll.goc.diff: Fix restricted word bug.
>   Rename variable errno to errno1.
> * src_libgo_go_os_os_test.go.diff: Allow EFBIG return code to big
> seeks.
> * src_libgo_go_syscall_syscall_gnu_test.go: New file:
>   Define Type and Whence as 32bit in syscall.Flock_t
> * src_libgo_testsuite_gotest.diff: Remove comm option for ps -o.
> * add-gnu-to-libgo-headers.diff:
>   Add gnu to +build entry in file headers included in the build.
>   FIXME:  <add remaining headers>

I can't find these in the patches you sent.



> Index: gcc-6-6.2.1-4.1/src/libgo/testsuite/gotest
> ===================================================================
> --- gcc-6-6.2.1-4.1.orig/src/libgo/testsuite/gotest
> +++ gcc-6-6.2.1-4.1/src/libgo/testsuite/gotest
> @@ -618,7 +618,11 @@ xno)
>               wait $pid
>               status=$?
>               if ! test -f gotest-timeout; then
> -                 sleeppid=`ps -o pid,ppid,comm | grep " $alarmpid " | grep 
> sleep | sed -e 's/ *\([0-9]*\) .*$/\1/'`
> +                 if test "$goos" = "gnu"; then
> +                     sleeppid=`ps -o pid,ppid | grep " $alarmpid " | grep 
> sleep | sed -e 's/ *\([0-9]*\) .*$/\1/'`
> +                 else
> +                     sleeppid=`ps -o pid,ppid,comm | grep " $alarmpid " | 
> grep sleep | sed -e 's/ *\([0-9]*\) .*$/\1/'`
> +                 fi
>                   kill $alarmpid
>                   wait $alarmpid
>                   if test "$sleeppid" != ""; then


We could rather just implement the comm field in ps, AIUI it's just an
alias for the command field.

Samuel