Go: x/sys/unix: Stat_t.Atim is named Atimespec on darwin

[guonaihong]

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

$ go version
go version go1.11.1 linux/amd64

Does this issue reproduce with the latest release?

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

go env Output

$ go env

GOARCH="amd64"

GOBIN=""

GOCACHE="/home/guo/.cache/go-build"

GOEXE=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="linux"

GOOS="linux"

GOPATH="/home/guo/go"

GOPROXY=""

GORACE=""

GOROOT="/home/guo/go-version/go1.11.1"

GOTMPDIR=""

GOTOOLDIR="/home/guo/go-version/go1.11.1/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-build480480625=/tmp/go-build -gno-record-gcc-switches"

What did you do?

cat test.go

package main

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

func statTimes(name string) (atime, mtime, ctime time.Time, err error) {
    var stat unix.Stat_t
    err = unix.Stat(name, &stat)
    if err != nil {
        return
    }

    atime = time.Unix(int64(stat.Atim.Sec), int64(stat.Atim.Nsec))
    mtime = time.Unix(int64(stat.Mtim.Sec), int64(stat.Mtim.Nsec))
    ctime = time.Unix(int64(stat.Ctim.Sec), int64(stat.Ctim.Nsec))
    return
}

func main() {

    fmt.Println(statTimes(os.Args[0]))
}

linux

env GOPATH=`pwd` CGO_ENABLED=0 GOOS=linux GOARCH=amd64  go run test.go

output

2019-04-29 09:16:40.531596746 +0800 CST 2019-04-29 09:16:40.52759687 +0800 CST 2019-04-29 09:16:40.52759687 +0800 CST <nil>

darwin

env GOPATH=`pwd` CGO_ENABLED=0 GOOS=darwin GOARCH=amd64  go run test.go

output

./test.go:17:30: stat.Atim undefined (type unix.Stat_t has no field or method Atim)
./test.go:18:30: stat.Mtim undefined (type unix.Stat_t has no field or method Mtim)
./test.go:19:30: stat.Ctim undefined (type unix.Stat_t has no field or method Ctim)

What did you expect to see?

Mac platform is the same as linux output

What did you see instead?

Mac platform error

[bradfitz]

@tklauser, @ianlancetaylor, IIRC this is named differently in x/sys/unix because it was in syscall? Is that enough of an argument? Renaming it would break more people, though.

I think the best option would probably be to add some accessors methods (both get & set) for those fields that are portable.

[ianlancetaylor]

The names in the syscall package vary on various systems.

js, nacl: two fields: Atime, AtimeNsec, both int64
aix: Atim with type StTimespec_t
darwin, freebsd, netbsd: Atimespec with type Timespec
dragonfly, linux, openbsd, solaris: Atim with type Timespec

The names in the x/sys/unix package also vary, and are slightly different:

aix: Atim with type StTimespec
darwin, netbsd: Atimespec with type Timespec
dragonfly, freebsd, linux, openbsd, solaris: Atim with type Timespec

freebsd/386 also has an Atim_ext field, I'm not sure what that is.

These are all of course a result of different paths that the operating systems have taken to expand the traditional st_atime field beyond holding just seconds.

Since the fields in x/sys/unix are all type Timespec or an equivalent, I think we could seriously consider renaming all of them to Atim with type Timespec. But accessor methods would also work. We would need six methods (get and set for each ofAtim,Mtim,Ctim`), but by using build tags we would only need three implementations.

[bradfitz]

I'd support a breaking change if you do. People using modules wouldn't get a surprise break until an update anyway.

[tklauser]

I'd be fine with a breaking change as well. FWIW, we already did the Atimespec -> Atim rename on freebsd (as part of the ino64support by @paulzhol in https://golang.org/cl/136816).

[paulzhol]

We've reverted syscall back to Atimespec in http://golang.org/cl/155958 though.

[bradfitz]

@paulzhol, std/syscall has stricter compatibility rules, though. We can do more cleanups in x/sys/unix.

[paulzhol]

@bradfitz it should be [ACM]time according to posix.
I'm just worried that we'd to end up with something like m4/stat-time.m4 and lib/stat-time.h the more systems we add.

Also I was under the assumption that with the syscall freeze, x/sys/unix would be a replacement which would allow more rapid changes independent of the standard compiler and library. Not to provide a unified low-level view of all unix systems.

[ianlancetaylor]

Yes, st_atim should work in C on all Unix systems. The problem is that Go is underlying at the underlying structs, and they have different names on different systems. C programmers don't normally see those names. It seems worthwhile to arrange for the x/sys/unix package to provide consistent names across systems, just as C programmers see. The only downside is breaking compatibility on some systems. I don't think this as taking a position on whether x/sys/unix should provide a unified low-level view of all Unix systems; it's just the appropriate response for this specific case.

[gopherbot]

Change https://golang.org/cl/175037 mentions this issue: unix: add portable Stat_t time accessors

[bradfitz]

Here's the other option of adding accessors instead of renaming fields: https://go-review.googlesource.com/c/sys/+/175037

Thoughts?

[paulzhol]

Can we pre-process cgo -godefs input with cpp with a _POSIX_C_SOURCE=1 to get the translated names in ztypes_*?
edit: nevermind, it might work for darwin, but at least on FreeBSD the field members are different regardless (st_atim). The _POSIX_C_SOURCE just defines the accessors to be at_atime or at_atimespec.

[tklauser]

Here's the other option of adding accessors instead of renaming fields: https://go-review.googlesource.com/c/sys/+/175037

Thoughts?

Nice. Now that I see how straightforward the implementation is, I slightly prefer this to renaming the fields. I think it is less brittle than some preprocessor magic and/or ugly regex based postprocessing in mkpost.go.

Also, this would in principle be backportable to package syscall.

[gopherbot]

Change https://golang.org/cl/175157 mentions this issue: unix: rename Stat_t time fields to [AMC]time and Birthtime

[paulzhol]

The rename version is at https://golang.org/cl/175157. Please run the try bots I'm sure stuff are broken.

[tklauser]

The rename version is at https://golang.org/cl/175157. Please run the try bots I'm sure stuff are broken.

TryBots seem happy but they wont test on many of the affected geese (e.g. most BSDs, solaris, aix).

[paulzhol]

I also think the accessors CL is elegant, but we already have two versions of it due to syscall differences:

https://github.com/golang/go/blob/master/src/os/stat_unix.go
https://github.com/golang/go/blob/master/src/os/stat_linux.go
https://github.com/golang/go/blob/master/src/os/stat_freebsd.go
plus many more

and

https://github.com/golang/go/blob/master/src/archive/tar/stat_unix.go
https://github.com/golang/go/blob/master/src/archive/tar/stat_actime1.go
https://github.com/golang/go/blob/master/src/archive/tar/stat_actime2.go

[bradfitz]

@paulzhol, we can ignore std for the purpose of deciding x/sys's API. If anything, perhaps we can even make std use x/sys/unix instead of std's syscall. Russ said he didn't want two syscall packages in std, but he never said the one couldn't be x/sys/unix. So perhaps we clean up x/sys/unix and then switch std to using it.

[ianlancetaylor]

Let's rename the fields to the names used on GNU/Linux.

[gopherbot]

Change https://golang.org/cl/177437 mentions this issue: unix: fix TestStatFieldNames on aix and TestUtimesNanoAt on darwin