Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/sys/unix: Select signature on BSDs omits integer return value, unlike Linux etc #34458

Closed
ThomasHabets opened this issue Sep 22, 2019 · 12 comments

Comments

@ThomasHabets
Copy link
Contributor

@ThomasHabets ThomasHabets commented Sep 22, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.1 openbsd/amd64

Does this issue reproduce with the latest release?

Yes. It's still in the code in HEAD.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/thompa/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="openbsd"
GOOS="openbsd"
GOPATH="/home/thompa/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/openbsd_amd64"
GCCGO="gccgo"
CC="cc"
CXX="c++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build396939324=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Tried to build this on OpenBSD, which builds fine on Linux:

package main

import (
	"fmt"
	"golang.org/x/sys/unix"
	"syscall"
)

type s func(n int, r, w, e *syscall.FdSet, timeout *syscall.Timeval) (int, error)
type s2 func(n int, r, w, e *unix.FdSet, timeout *unix.Timeval) (int, error)

func main() {
	var t s
	t = syscall.Select

	var t2 s2
	t2 = unix.Select

	fmt.Println("Hello, playground", t, t2)

}

What did you expect to see?

Since OpenBSD has the same POSIX semantics for Select, as far as I know, as Linux, I expect the same function prototype and for the example above to successfully build.

What did you see instead?

On OpenBSD (and Mac, and all BSDs?) the above fails to build because syscall.Select and unix.Select only return error, not (int, error).

$ go build t.go
# command-line-arguments
./t.go:14:4: cannot use syscall.Select (type func(int, *syscall.FdSet, *syscall.FdSet, *syscall.FdSet, *syscall.Timeval) error) as type s in assignment
./t.go:17:5: cannot use unix.Select (type func(int, *unix.FdSet, *unix.FdSet, *unix.FdSet, *unix.Timeval) error) as type s2 in assignment

How can I make portable programs when Go removes my ability to call Select?

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Sep 23, 2019

CC @tklauser

This part of the syscall API appears to date to CL 29723, before Go's open-source release, and the first place it appears is in syscall_darwin.go.

Is it possible that the Darwin version of this syscall does not return the number of file descriptors from the kernel, and instead computes it on the caller side? What about the other *BSDs?

@ThomasHabets

This comment has been minimized.

Copy link
Contributor Author

@ThomasHabets ThomasHabets commented Sep 23, 2019

OpenBSD certainly claims to "select() returns the total number of ready descriptors in all the sets".

And looking at kernel code it looks like yes.

@tklauser

This comment has been minimized.

Copy link
Member

@tklauser tklauser commented Sep 23, 2019

OpenBSD certainly claims to "select() returns the total number of ready descriptors in all the sets".

The same seems to be true for Darwin (didn't find an online source for this, checked locally on macOS 10.14), FreeBSD, NetBSD and DragonflyBSD. Probably also Solaris and Illumos.

On Darwin I also checked that unix.Select returns the correct number of ready file descriptors (both with the Go1.12+ libSystem syscalls and the Go1.11 raw syscalls) using the following addition to TestSelect:

        rr, ww, err := os.Pipe()
        if err != nil {
                t.Fatal(err)
        }
        defer rr.Close()
        defer ww.Close()

        if _, err := ww.Write([]byte("HELLO GOPHER")); err != nil {
                t.Fatal(err)
        }

        rFdSet := &unix.FdSet{}
        fd := rr.Fd()
        // FD_SET(fd, rFdSet), this needs NFDBITS from sys/select.h as a generated const
        rFdSet.Bits[fd/unix.NFDBITS] |= (1 << (fd % unix.NFDBITS))

        n, err := unix.Select(int(fd+1), rFdSet, nil, nil, nil)
        if err != nil {
                t.Fatalf("Select: %v", err)
        }
        if n != 1 {
                t.Fatalf("Select: expected 1 ready file descriptors, got %v", n)
        }

How would we go about changing Select? I guess it should be no problem in x/sys/unix as it is using modules. I'm not sure about syscall.

/cc @bradfitz @ianlancetaylor

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Sep 23, 2019

How can I make portable programs when Go removes my ability to call Select?

The default answer to that is:

"Syscall must always be guarded with build tags." -- https://go-proverbs.github.io

That said, this signature should probably match everywhere.

Let's just fix x/sys/unix but not syscall.

@bradfitz bradfitz changed the title x/sys: syscall.Select() and x/sys/unix.Select() don't return number of fds on BSDs x/sys/unix: Select signature on BSDs omits integer return value, unlike Linux etc Sep 23, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 23, 2019

Change https://golang.org/cl/196802 mentions this issue: unix: fix Select to return number of ready fds on Darwin and *BSD

@ThomasHabets

This comment has been minimized.

Copy link
Contributor Author

@ThomasHabets ThomasHabets commented Sep 23, 2019

Right. Yeah fixing syscall will break anyone doing build tags for this today. Out of curiosity, is there a policy for breaking changes to syscall?

@ThomasHabets

This comment has been minimized.

Copy link
Contributor Author

@ThomasHabets ThomasHabets commented Sep 23, 2019

And exposing NFDBITS would be helpful too.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 24, 2019

We are not going to make a breaking change to the syscall package. We can do that if absolutely necessary, but this is not absolutely necessary.

We can change x/sys/unix for a good enough reason.

@tklauser

This comment has been minimized.

Copy link
Member

@tklauser tklauser commented Sep 24, 2019

And exposing NFDBITS would be helpful too.

https://golang.org/cl/196802 will add NFDBITSfor Darwin and the BSDs. I'll send a follow-up CL adding it for Linux etc.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 24, 2019

Change https://golang.org/cl/196805 mentions this issue: unix: re-generate Select on dragonfly

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 24, 2019

Change https://golang.org/cl/196806 mentions this issue: unix: add NFDBITS const on Linux, update TestSelect

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 24, 2019

Change https://golang.org/cl/196807 mentions this issue: unix: fix Select to return number of ready fds on Solaris

gopherbot pushed a commit to golang/sys that referenced this issue Sep 24, 2019
CL 196802 did not properly re-generate the definition of Select after
changing the //sys comment.

Updates golang/go#34458

Change-Id: I035468487163f48393fc777dde691737fce41aa8
Reviewed-on: https://go-review.googlesource.com/c/sys/+/196805
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit to golang/sys that referenced this issue Sep 24, 2019
Make Select's signature on Solaris match the one on Linux and return the number
of ready file descriptors.

Updates golang/go#34458

Change-Id: I118c4c35cbc83dba015ef357ce9bef44c9165ec1
Reviewed-on: https://go-review.googlesource.com/c/sys/+/196807
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit to golang/sys that referenced this issue Sep 24, 2019
Adjust TestSelect on Linux to match the Darwin/*BSD changes in CL
196802.

Updates golang/go#34458

Change-Id: Ia17fdadf7091001ea785391da23aaf9d3ec4ac5e
Reviewed-on: https://go-review.googlesource.com/c/sys/+/196806
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.