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

[Samuel Thibault]

Samuel Thibault, on Mon 19 Dec 2016 00:25:35 +0100, wrote:
> as the attached patch does, which should really be applied or done
> any other way.

Or rather this patch, which makes it more like the test above.

Matthias, I'm committing this to Debian's gcc-6, along the other go
patches from Svante.

Samuel
Index: gcc/src/libgo/runtime/go-caller.c
===
--- gcc/src/libgo/runtime/go-caller.c   (révision 235086)
+++ gcc/src/libgo/runtime/go-caller.c   (copie de travail)
@@ -93,7 +93,7 @@
 argv[0] (http://gcc.gnu.org/PR61895).  It would be nice to
 have a better check for whether this file is the real
 executable.  */
-   if (stat (filename, ) < 0 || s.st_size < 1024)
+   if (filename != NULL && (stat (filename, ) < 0 || s.st_size < 1024))
filename = NULL;
 
   back_state = backtrace_create_state (filename, 1, error_callback, NULL);

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

[Samuel Thibault]

Hello,

Svante Signell, on Fri 25 Nov 2016 20:57:26 +0100, wrote:
> Another more annoying gnumch/hurd/glibc bug is that the
> built program go (go-6 in Debian) gets killed when executed from the
> shell vi path, but not when issued directly: /usr/bin/go-6 works fine.
>  go-6
> Segmentation fault (core dumped)

I've had a quick look by adding printfs in go-main.c and further to see
up to where it goes before crashing.  It crashes in

src/libgo/runtime/go-caller.c

in function __go_get_backtrace_state, inside the stat() call in:

if (__builtin_strchr (filename, '/') == NULL)
  filename = NULL;

if (stat (filename, ) < 0 || s.st_size < 1024)
  filename = NULL;

filename of course doesn't start with '/' (it's argv[0]), and thus NULL
is passed to stat(). It happens that by luck on Linux this just returns
an EFAULT error, but that's sheer luck :) This should probably be turned
into e.g.:

if (!filename || stat (filename, ) < 0 || s.st_size < 1024)
  filename = NULL;

as the attached patch does, which should really be applied or done
any other way.



Then calling go-6 brings this:

  fatal error: libbacktrace could not find executable to open

That's inside src/libbacktrace/fileline.c, the fileline_initialize
function, which tries the above filename (and thus fails), then
getexecname() (which is not implemented), then /proc/self/exe, which is
not implemented, then /proc/curproc/file which is even less implemented.

So here it'd be "just" a matter of implementing /proc/self/exe.

Samuel
Index: runtime/go-caller.c
===
--- runtime/go-caller.c (révision 235086)
+++ runtime/go-caller.c (copie de travail)
@@ -93,7 +93,7 @@
 argv[0] (http://gcc.gnu.org/PR61895).  It would be nice to
 have a better check for whether this file is the real
 executable.  */
-  if (stat (filename, ) < 0 || s.st_size < 1024)
+  if (!filename || stat (filename, ) < 0 || s.st_size < 1024)
filename = NULL;
 
   back_state = backtrace_create_state (filename, 1, error_callback, NULL);

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

[Ian Lance Taylor]

On Wed, Dec 7, 2016 at 2:02 PM, Svante Signell  wrote:
>
> I'm sorry but neither of you or I are in the position to request changes of
> already existing file names e.g. replacing linux with glibc in:
> os/pipe_linux.go
> crypto/x509/root_linux.go
> syscall/errstr_linux.go

As you know, the name "linux" in those files means that the files are
only compiled on GNU/Linux systems.  The files in os and crypto are
copied from the master library in the gc repository.  Those names are
not going to change.  The file syscall/errstr_linux.go is specific to
gccgo, and that name could change easily enough.

> Neither are we in the position to request to rename libcall_posix.go to
> something else, or move the four functions definitions mount, mlocall,
> munlockall, madvise, mentioned above to a separate file.

Making those changes is fine.

> So far, nobody, upstream or debian-gcc, have made any comment on the patches
> whatsoever. So what are we doing? Let's forget about porting gccgo to 
> GNU/Hurd.

Yes, I'm sorry that this never gets high enough on my list for a serious review.

The best way to contribute to gccgo is to use the process described at
https://golang.org/doc/gccgo_contribute.html.  That explains how to
send changes to Gerrit in a way that is easy for me to review and
apply (Gerrit is itself free software).  You are much more likely to
get a response if you are able to follow that approach.

Ian

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

[Svante Signell]

On Wed, 2016-12-07 at 17:25 +0100, Samuel Thibault wrote:
> Svante Signell, on Wed 07 Dec 2016 15:32:31 +0100, wrote:
> > On Wed, 2016-12-07 at 15:08 +0100, Samuel Thibault wrote:
> > > Ok, but then I'd say move the function which change to a separate file,
> > > so that the other functions are kept shared.
> > > Otherwise it'll be tedious to maintain.
> > 
> > One problem is the file go/syscall/libcall_posix-1.go, derived
> > from go/syscall/libcall_posix.go but _removing_ four function definitions:
> > mount, madvise, mlockall and munlockall.
> 
> That's not a problem: move them out to another file, and include that
> file in the non-gnu case, but do not include that file in the gnu case.
> 
> Actually it's even odd to see mount among them, it's not a standard
> posix interface.

I'm sorry but neither of you or I are in the position to request changes of
already existing file names e.g. replacing linux with glibc in:
os/pipe_linux.go
crypto/x509/root_linux.go
syscall/errstr_linux.go

Neither are we in the position to request to rename libcall_posix.go to
something else, or move the four functions definitions mount, mlocall,
munlockall, madvise, mentioned above to a separate file.

So far, nobody, upstream or debian-gcc, have made any comment on the patches
whatsoever. So what are we doing? Let's forget about porting gccgo to GNU/Hurd.

Thanks!

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

[Samuel Thibault]

Svante Signell, on Wed 07 Dec 2016 15:32:31 +0100, wrote:
> On Wed, 2016-12-07 at 15:08 +0100, Samuel Thibault wrote:
> > Ok, but then I'd say move the function which change to a separate file,
> > so that the other functions are kept shared.
> > Otherwise it'll be tedious to maintain.
> 
> One problem is the file go/syscall/libcall_posix-1.go, derived
> from go/syscall/libcall_posix.go but _removing_ four function definitions:
> mount, madvise, mlockall and munlockall.

That's not a problem: move them out to another file, and include that
file in the non-gnu case, but do not include that file in the gnu case.

Actually it's even odd to see mount among them, it's not a standard
posix interface.

Samuel

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

[Svante Signell]

On Wed, 2016-12-07 at 15:08 +0100, Samuel Thibault wrote:
> Svante Signell, on Wed 07 Dec 2016 14:52:35 +0100, wrote:
> > Since go does not have a preprocessor allowing conditional code paths this
> > is
> > how it should be done (and as I did):
> > http://blog.ralch.com/tutorial/golang-conditional-compilation/
> 
> Ok, but then I'd say move the function which change to a separate file,
> so that the other functions are kept shared.
> Otherwise it'll be tedious to maintain.

One problem is the file go/syscall/libcall_posix-1.go, derived
from go/syscall/libcall_posix.go but _removing_ four function definitions:
mount, madvise, mlockall and munlockall. A possible solution would then be to
rename that file to libcall_posix_gnu.go. The file libcall_gnu.go name is
already used.

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

[Samuel Thibault]

Svante Signell, on Wed 07 Dec 2016 14:52:35 +0100, wrote:
> Since go does not have a preprocessor allowing conditional code paths this is
> how it should be done (and as I did):
> http://blog.ralch.com/tutorial/golang-conditional-compilation/

Ok, but then I'd say move the function which change to a separate file,
so that the other functions are kept shared.
Otherwise it'll be tedious to maintain.

Samuel

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

[Svante Signell]

On Sun, 2016-11-27 at 18:02 +0100, Samuel Thibault wrote:
> Hello,
> 
> Svante Signell, on Sun 27 Nov 2016 17:33:52 +0100, wrote:
> > > > 
> > > 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 found the answer, see below!

> > 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.
> 
> Then ask upstream how they think it can and should be done.
> 

Since go does not have a preprocessor allowing conditional code paths this is
how it should be done (and as I did):
http://blog.ralch.com/tutorial/golang-conditional-compilation/

Verdict
===
We should aim to develop and build our Go applications by following Go idioms.
If the source file targets a specific platform, we should choose file suffix
approach. Otherwise, if the source file is applicable for multiple platforms and
we want to exclude a specific feature or platform, we should use build
constraints instead.

Build constraints
=
A build constraints (known as build tags) is an optional top line comment that
starts with

// +build

package api

File suffixes
=
The second option for conditional compilation is the name of the
source file itself. This approach is simpler than build tags, and allows
the Go build system to exclude files without having to process the file.

We should add one of the following suffixes to desired files:

*_GOOS // operation system
*_GOARCH // platform architecture
*_GOOS_GOARCH // both combined

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

[Samuel Thibault]

Svante Signell, on Sun 27 Nov 2016 18:17:17 +0100, wrote:
> On Sun, 2016-11-27 at 18:02 +0100, Samuel Thibault wrote:
> > > But as you wish, an updated patch is attached.
> > 
> >  _Bool
> >  Continued (uint32_t *w)
> >  {
> > +#ifndef WCONTINUED
> > +  *w = 0;
> > +  return 0;
> > +#else
> >    return WIFCONTINUED (*w) != 0;
> > +#endif
> >  }
> > 
> > Err, recheck the semantic of WCONTINUED again, it doesn't modify its
> > parameter, it just tests its value.
> > 
> > Do as I said, just return 0.
> 
> No I can't the compiler complains about an unused variable. Maybe
> adding an __attribute__((unused)) to the function header?

For instance.  See how it is done in the rest of the code.

> > > This is for upstream to decide.
> > 
> > I'm just afraid they'd just frown on the code submission and not take
> > the time to explain how they want it to look like if we don't raise
> > the
> > discussion ourselves.
> > 
> 
> Should we propose these changes upstream? Or do you mean something
> else? 

I mean proposing explicitly to upstream, yes, so they don't have to
take the time to explain.

> > Then ask upstream how they think it can and should be done.
> 
> Upstream would be Ian Lance Taylor, right?

I don't know.

> > > > > -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. 
> > 
> > That's still unrelated to the matter here: porting go to
> > GNU/Hurd.  It
> > looks like a bug fix which is completely independant from GNU/Hurd.
> 
> Yes it is not Hurd-related. Maybe this should be filed as a separate
> bug. To gcc upstream directly?

Yes.

> > > > 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.
> > 
> > Then please do implement it :)
> 
> Sorry, I need help for doing this. Any other Hurd developer listening?

See spec_abbrevs in utils/ps.c, I guess it's a matter of adding
{"Comm=args"} in the list.

Samuel

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

[Svante Signell]

On Sun, 2016-11-27 at 18:02 +0100, Samuel Thibault wrote:
> Hello,
...
> > But as you wish, an updated patch is attached.
> 
>  _Bool
>  Continued (uint32_t *w)
>  {
> +#ifndef WCONTINUED
> +  *w = 0;
> +  return 0;
> +#else
>    return WIFCONTINUED (*w) != 0;
> +#endif
>  }
> 
> Err, recheck the semantic of WCONTINUED again, it doesn't modify its
> parameter, it just tests its value.
> 
> Do as I said, just return 0.
> 

No I can't the compiler complains about an unused variable. Maybe
adding an __attribute__((unused)) to the function header?

...

> 
> > This is for upstream to decide.
> 
> I'm just afraid they'd just frown on the code submission and not take
> the time to explain how they want it to look like if we don't raise
> the
> discussion ourselves.
> 

Should we propose these changes upstream? Or do you mean something
else? 

> > > 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.
> 
> Then ask upstream how they think it can and should be done.
> 

Upstream would be Ian Lance Taylor, right?

> > > > -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. 
> 
> That's still unrelated to the matter here: porting go to
> GNU/Hurd.  It
> looks like a bug fix which is completely independant from GNU/Hurd.

Yes it is not Hurd-related. Maybe this should be filed as a separate
bug. To gcc upstream directly?

> > > 
> > > 
> > > 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.
> 
> Then please do implement it :)

Sorry, I need help for doing this. Any other Hurd developer listening?

Thanks for reviewing :) Nice to see that anybody cares.

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

[Samuel Thibault]

Hello,

Svante Signell, on Sun 27 Nov 2016 17:33:52 +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 
> > > +#ifndef WCONTINUED
> > > +#define WCONTINUED 0
> > > +#endif
> > >  #include 
> > >  
> > >  #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.

"no visible differences" doesn't mean there is none :)

> But as you wish, an updated patch is attached.

 _Bool
 Continued (uint32_t *w)
 {
+#ifndef WCONTINUED
+  *w = 0;
+  return 0;
+#else
   return WIFCONTINUED (*w) != 0;
+#endif
 }

Err, recheck the semantic of WCONTINUED again, it doesn't modify its
parameter, it just tests its value.

Do as I said, just return 0.



> This is for upstream to decide.

I'm just afraid they'd just frown on the code submission and not take
the time to explain how they want it to look like if we don't raise the
discussion ourselves.



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

Then ask upstream how they think it can and should be done.



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

That's still unrelated to the matter here: porting go to GNU/Hurd.  It
looks like a bug fix which is completely independant from GNU/Hurd.

> > 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:  
> > 
> > I can't find these in the patches you sent.
> 
> It is in the last mail: patch 19-23(23)

Ah, sorry, mutt had pasted the content of the attached files without
putting the file names.

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

Then please do implement it :)

Samuel