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

[Svante Signell]

On Sun, 2016-11-27 at 00:17 +0100, Samuel Thibault wrote:
> Hello,
> 
> Nice work!

Thanks :)

> 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.

It's just for convenience of handling. It's a simple matter to collapse
them all into one patch.
 
> > 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?!

Of course not. The comment said: Update accordingly with
autoreconf2.64. Additionally, automake1.11 has to be installed, and no
newer automake version can be installed to regenerate that file after a
change in Makefile.am. The build system is unfortunately based on these
old auto-tools. A change to later versions of automake and autoconf
would be preferred. However, this is an effort (probably not trivial)
that has to be made upstream for the whole gcc-* development.

> 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.
> 

I've been implementing that version too, with no visible differences.
But as you wish, an updated patch is attached.

> > @@ -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
> 

That's for upstream to decide.

> > @@ -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.
> 

See above about naming. I'll remove the go/net/sendfile_gnu.go file
from the patches.

> > +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?
> 

Sorry, I thought I had removed duplicates. New Makefile.am,in and
debian_rules.patch patches will be attached in a subsequent mail.

> > @@ -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...
> 

This is for upstream to decide. And see below about making conditional
constructs in go-code.

> 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.
>

I tried to patch the syscall_unix_test.go file, but did not succeed. 
Definitely if runtime.GOOS == "GNU" ... else ... or switch runtime.GOOS
... does not work. The compiler sees all code and complains, also the
else part of the code :( Therefore I created a new 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?
> 
No, this is not unrelated: With this patch you can
make -C build/i686-gnu/libgo clean
make -C build/i686-gnu/libgo
to rebuild libgo. Otherwise libcalls.go is not regenerated,
mksysinfo.sh is not run etc. 

> > 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
> 

I thought I had cleaned this up right now, but obviously not. New patch
applied.

> 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.
> 

It is in the last mail: patch 19-23(23)
As the comment said, more files have to be patched to add gnu to the
+build header field.

> > 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.

Your choice. When implemented this patch wouldn't bee needed.

src_libgo_go_syscall_wait.c.diff
Description: Text Data

src_libgo_mksysinfo.sh.diff
Description: Text Data