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

syscall: Under FreeBSD Stat_t field names (eg Ctimespec) have changed in a backwards incompatible way #29393

Closed
ncw opened this issue Dec 22, 2018 · 8 comments

Comments

@ncw
Copy link
Contributor

@ncw ncw commented Dec 22, 2018

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

go version devel +97d5cb24b1 Sat Dec 22 09:37:04 2018 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ncw/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ncw/go"
GOPROXY=""
GORACE=""
GOROOT="/opt/go/go-tip"
GOTMPDIR=""
GOTOOLDIR="/opt/go/go-tip/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build103132839=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I tried to run the compilation tests of https://github.com/a8m/tree to find out why it wasn't compiling under go1.12beta1

What did you expect to see?

I expected it to compile

What did you see instead?

Try GOOS=freebsd GOARCH=386
# github.com/a8m/tree
./csort_bsd.go:17:11: s1.Ctimespec undefined (type *syscall.Stat_t has no field or method Ctimespec)
./csort_bsd.go:17:30: s2.Ctimespec undefined (type *syscall.Stat_t has no field or method Ctimespec)
*** Failed compiling GOOS=freebsd GOARCH=386
Try GOOS=freebsd GOARCH=amd64
# github.com/a8m/tree
./csort_bsd.go:17:11: s1.Ctimespec undefined (type *syscall.Stat_t has no field or method Ctimespec)
./csort_bsd.go:17:30: s2.Ctimespec undefined (type *syscall.Stat_t has no field or method Ctimespec)
*** Failed compiling GOOS=freebsd GOARCH=amd64
Try GOOS=freebsd GOARCH=arm
# github.com/a8m/tree
./csort_bsd.go:17:11: s1.Ctimespec undefined (type *syscall.Stat_t has no field or method Ctimespec)
./csort_bsd.go:17:30: s2.Ctimespec undefined (type *syscall.Stat_t has no field or method Ctimespec)
*** Failed compiling GOOS=freebsd GOARCH=arm

None of the FreeBSD architectures compiled.

Digging into this I can see the Stat_t type has changed from go1.11 to go1.12beta1

$ diff -u -w -b /tmp/go1.11 /tmp/go1.12beta1 
--- /tmp/go1.11	2018-12-22 10:49:12.713455762 +0000
+++ /tmp/go1.12beta1	2018-12-22 10:49:01.097378144 +0000
@@ -1,19 +1,22 @@
 type Stat_t struct {
-	Dev           uint32
-	Ino           uint32
+	Dev      uint64
+	Ino      uint64
+	Nlink    uint64
 	Mode          uint16
-	Nlink         uint16
+	Padding0 int16
 	Uid           uint32
 	Gid           uint32
-	Rdev          uint32
-	Atimespec     Timespec
-	Mtimespec     Timespec
-	Ctimespec     Timespec
+	Padding1 int32
+	Rdev     uint64
+	Atim     Timespec
+	Mtim     Timespec
+	Ctim     Timespec
+	Birthtim Timespec
 	Size          int64
 	Blocks        int64
-	Blksize       uint32
+	Blksize  int32
 	Flags         uint32
-	Gen           uint32
-	Lspare        int32
-	Birthtimespec Timespec
+	Gen      uint64
+	Spare    [10]uint64
 }

This is mentioned in the draft release notes

64-bit inodes are now supported on FreeBSD 12. Some types have been adjusted accordingly.

The changes to the types seem reasonable, however the change to the names of Atimespec, Mtimespec, Ctimespec seem like gratuitous breaking changes - the types are the same as previously, if the names had been left the same then the program above would have compiled just fine!

Can these be changed back?

@ncw
Copy link
Contributor Author

@ncw ncw commented Dec 22, 2018

The breaking change is mentioned in dc6eb20

Breaking API changes in package syscall are:
Mknod takes a uint64 (C dev_t) instead of int.
Stat_t: Dev, Ino, Nlink, Rdev, Gen became uint64.
Atimespec, Mtimespec, Ctimespec, Birthtimespec renamed to Atim, Mtim, Ctim, Birthtim respectively.

I see there was some discussion of this in https://go-review.googlesource.com/c/go/+/138595

If we have to do stuff like this ourselves, I worry how much stuff in the wild will break with this change.

;-)

@ncw
Copy link
Contributor Author

@ncw ncw commented Dec 22, 2018

I made a patch to a8m/tree to see what it looked like to work around this - it turns out it isn't too ugly. Renaming the Ctimespec field to Ctim does at least bring it into line with some of the other unixes, even if it diverges from the other BSDs

diff --git a/csort_bsd.go b/csort_bsd.go
index f5895a6..50a769b 100644
--- a/csort_bsd.go
+++ b/csort_bsd.go
@@ -1,4 +1,4 @@
-//+build darwin freebsd netbsd
+//+build darwin freebsd,!go1.12 netbsd
 
 package tree
 
diff --git a/csort_unix.go b/csort_unix.go
index 6acd092..eead420 100644
--- a/csort_unix.go
+++ b/csort_unix.go
@@ -1,4 +1,4 @@
-//+build linux openbsd dragonfly android solaris
+//+build linux openbsd dragonfly android solaris freebsd,go1.12
 
 package tree
 
ncw added a commit to a8m/tree that referenced this issue Dec 22, 2018
See discussion in golang/go#29393 also.
@paulzhol
Copy link
Member

@paulzhol paulzhol commented Dec 23, 2018

These names were introduced more than 8 years ago:
freebsd/freebsd@4f08ecd

And the syscall structs are semi-automatically generated from the C headers.

@paulzhol paulzhol added the OS-FreeBSD label Dec 23, 2018
@ncw
Copy link
Contributor Author

@ncw ncw commented Dec 28, 2018

According to the go compatibility promise

It is impossible to guarantee long-term compatibility with operating system interfaces, which are changed by outside parties. The syscall package is therefore outside the purview of the guarantees made here. As of Go version 1.4, the syscall package is frozen. Any evolution of the system call interface must be supported elsewhere, such as in the go.sys subrepository. For details and background, see this document.

I read this as the syscall package is frozen from go 1.4 so backwards incompatible changes should not be introduced unless absolutely necessary.

Personally speaking, I'm easy either way, but I'd like this issue to have a statement from the go team so when go1.12 is released and it breaks people's programs, they will have have some rationale to read.

Cc: @bradfitz since he reviewed the original CL.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 28, 2018

It's not actually clear to me why we changed the names of the Ctimespec, Mtimespec, and Atimespec fields. It would make sense to change the names if the types had to change, but comparing 1.11 and tip it seems that the types are the same. Is there any reason not to keep the field names we had all along? It doesn't seem to me that the fact that the fields changed names in FreeBSD is enough to reason to change the fields in the syscall package. That would be a reason to change the field in the golang.org/x/sys/unix package, which is what all new code should be using anyhow.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 28, 2018

I'm slightly in favor of renaming them back.

But since some fields are necessarily changing anyway, I didn't fight too much breaking some other fields while we're at it. (People who use syscall are due for some pain in any case.) But we could reduce some of it here.

@paulzhol paulzhol self-assigned this Dec 29, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Dec 29, 2018

Change https://golang.org/cl/155958 mentions this issue: syscall: revert to pre-FreeBSD 10 / POSIX-2008 timespec field names in Stat_t on FreeBSD

@paulzhol
Copy link
Member

@paulzhol paulzhol commented Dec 29, 2018

@ianlancetaylor we were carrying our own hard-coded version of struct stat8, based on the 8-STABLE version since go1.3 (to support FreeBSD 10 I think, discussion).
And since the go1.4 syscall freeze it was kept unmodified.

FreeBSD 12 introduces a struct freebsd11_stat struct for backward compatibility with pre-64bit inode syscalls. it, and the FreeBSD 12 version of struct stat use the new names.

I've sent a CL to rewrite them back using mkpost.go.

@gopherbot gopherbot closed this in 480373c Dec 30, 2018
@golang golang locked and limited conversation to collaborators Dec 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.