x/sys/unix: expose padding fields on s390x or hide them on other architectures #18632
Comments
|
Sorry we made you the guinea pig, but I do think it was for the best. It let us find and clean stuff like pkg/sftp@4d0e916. I'd like to see if we could remove those fields from all architectures if we can fix all projects depending on them first. |
|
Paging Mr. BigQuery @campoy to grep github. |
|
It's probably worth mentioning that both these cases involve the syscall package, not x/sys. It may be worth checking for uses of |
|
I think the old sftp code was correct.
The statfs_t struct on Linux does have fsid field, and its type fsid_t,
does contain two uint32_t vals. All of this is documented in statfs(2)
manual page.
I'm not sure about other cases, but at least for Fsid_t, the s390x
port is wrong to remove the field. (The field shouldn't be named
like a padding field, but that's another issue and we can't fix now.)
If we decided to remove all X_ fields, we should careful review
to avoid making the same mistake.
|
|
@minux You're right that fsid is a real part of the API, although the way pkg/sftp was encoding the field was probably incorrect for big endian systems. It's also not clear that it is a useful field, but really that's for the package maintainers to decide, not me. We could rename the As for careful review: I think it would be reasonable (there are 28 blanked out fields across 15 structs) to introduce tests for the structs we modify when they are expected to expose specific fields across every platform. Test coverage for x/sys/unix is currently only about 14% (measured on linux/s390x), we should aim to improve that at least a little as part of addressing this issue. |
|
fsid is an opaque value, so as long as it's used only to identify a file
system in a single process, endianness doesn't matter.
Even if the syscall is locked down, we should still fix the s390x bug.
|
|
FWIW there is an abandoned CL to expose |
mkpost.go replaces all variables prefixed with 'X_' with '_' on s390x because most of them do not need to be exposed. X__val is being used by a third party library so it turns out we do need to expose it on s390x (it is already exposed on all other Linux architectures). Fixes #17298 and updates #18632. Change-Id: Ic03463229a5f75ca41a4a4b50300da4b4d892d45 Reviewed-on: https://go-review.googlesource.com/30130 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
|
Should we also do a complete audit of similar accidentally hidden
useful fields on the s390x port before Go 1.8?
|
|
Below is the diff that re-adding the padding fields makes to the syscall package. I would say the ones that should maybe be added to syscall (probably not x/sys) for 1.8 are:
Everything else looks ok to me, but please take a look. @@ -31,13 +31,13 @@ type Timeval struct {
type Timex struct {
Modes uint32
- _ [4]byte
+ Pad_cgo_0 [4]byte
Offset int64
Freq int64
Maxerror int64
Esterror int64
Status int32
- _ [4]byte
+ Pad_cgo_1 [4]byte
Constant int64
Precision int64
Tolerance int64
@@ -46,14 +46,14 @@ type Timex struct {
Ppsfreq int64
Jitter int64
Shift int32
- _ [4]byte
+ Pad_cgo_2 [4]byte
Stabil int64
Jitcnt int64
Calcnt int64
Errcnt int64
Stbcnt int64
Tai int32
- _ [44]byte
+ Pad_cgo_3 [44]byte
}
type Time_t int64
@@ -103,7 +103,7 @@ type Stat_t struct {
Mode uint32
Uid uint32
Gid uint32
- _ int32
+ X__glibc_reserved0 int32
Rdev uint64
Size int64
Atim Timespec
@@ -111,7 +111,7 @@ type Stat_t struct {
Ctim Timespec
Blksize int64
Blocks int64
- _ [3]int64
+ X__glibc_reserved [3]int64
}
type Statfs_t struct {
@@ -127,7 +127,7 @@ type Statfs_t struct {
Frsize uint32
Flags uint32
Spare [4]uint32
- _ [4]byte
+ Pad_cgo_0 [4]byte
}
type Dirent struct {
@@ -136,7 +136,7 @@ type Dirent struct {
Reclen uint16
Type uint8
Name [256]uint8
- _ [5]byte
+ Pad_cgo_0 [5]byte
}
type Fsid struct {
@@ -146,11 +146,11 @@ type Fsid struct {
type Flock_t struct {
Type int16
Whence int16
- _ [4]byte
+ Pad_cgo_0 [4]byte
Start int64
Len int64
Pid int32
- _ [4]byte
+ Pad_cgo_1 [4]byte
}
type RawSockaddrInet4 struct {
@@ -231,13 +231,13 @@ type IPv6Mreq struct {
type Msghdr struct {
Name *byte
Namelen uint32
- _ [4]byte
+ Pad_cgo_0 [4]byte
Iov *Iovec
Iovlen uint64
Control *byte
Controllen uint64
Flags int32
- _ [4]byte
+ Pad_cgo_1 [4]byte
}
type Cmsghdr struct {
@@ -279,7 +279,7 @@ type TCPInfo struct {
Probes uint8
Backoff uint8
Options uint8
- _ [2]byte
+ Pad_cgo_0 [2]byte
Rto uint32
Ato uint32
Snd_mss uint32
@@ -450,7 +450,7 @@ type RtAttr struct {
type IfInfomsg struct {
Family uint8
- _ uint8
+ X__ifi_pad uint8
Type uint16
Index int32
Flags uint32
@@ -498,7 +498,7 @@ type SockFilter struct {
type SockFprog struct {
Len uint16
- _ [6]byte
+ Pad_cgo_0 [6]byte
Filter *SockFilter
}
@@ -528,21 +528,21 @@ type PtracePsw struct {
type PtraceFpregs struct {
Fpc uint32
- _ [4]byte
+ Pad_cgo_0 [4]byte
Fprs [16]float64
}
type PtracePer struct {
Control_regs [0]uint64
- _ [24]byte
- _ [8]byte
+ Pad_cgo_0 [24]byte
+ Pad_cgo_1 [8]byte
Starting_addr uint64
Ending_addr uint64
Perc_atmid uint16
- _ [6]byte
+ Pad_cgo_2 [6]byte
Address uint64
Access_id uint8
- _ [7]byte
+ Pad_cgo_3 [7]byte
}
type FdSet struct {
@@ -560,12 +560,12 @@ type Sysinfo_t struct {
Freeswap uint64
Procs uint16
Pad uint16
- _ [4]byte
+ Pad_cgo_0 [4]byte
Totalhigh uint64
Freehigh uint64
Unit uint32
- _ [0]uint8
- _ [4]byte
+ X_f [0]uint8
+ Pad_cgo_1 [4]byte
}
type Utsname struct {
@@ -579,16 +579,16 @@ type Utsname struct {
type Ustat_t struct {
Tfree int32
- _ [4]byte
+ Pad_cgo_0 [4]byte
Tinode uint64
Fname [6]uint8
Fpack [6]uint8
- _ [4]byte
+ Pad_cgo_1 [4]byte
}
type EpollEvent struct {
Events uint32
- _ int32
+ X_padFd int32
Fd int32
Pad int32
}
@@ -606,7 +606,7 @@ type Termios struct {
Lflag uint32
Line uint8
Cc [32]uint8
- _ [3]byte
+ Pad_cgo_0 [3]byte
Ispeed uint32
Ospeed uint32
} |
|
CL https://golang.org/cl/35262 mentions this issue. |
|
@mundaym, can we close this? |
|
Yeah I think we can close it if we're happy to maintain the status quo and leave the padding fields in the API exposed on non-s390x platforms. I certainly don't think it is worth adding them wholesale to the s390x API at this point. |
|
Change https://golang.org/cl/89495 mentions this issue: |
Fixes golang/go#18632. Change-Id: I9d07937dc93f860b09a99a86fe517b85f4dcd1b2 Reviewed-on: https://go-review.googlesource.com/89495 Reviewed-by: Ian Lance Taylor <iant@golang.org> Run-TryBot: Ian Lance Taylor <iant@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
|
Change https://golang.org/cl/100556 mentions this issue: |
|
Change https://golang.org/cl/100595 mentions this issue: |
Re-run ./mkall.sh to avoid exporting Pad_cgo* fields on solaris/amd64. Akin to CL 89495. Updates golang/go#18632 Change-Id: Ib80bf6376d0b7c0a5efd0f5d4a0f27e8e37d8abe Reviewed-on: https://go-review.googlesource.com/100556 Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
Re-run ./mkall.sh to avoid exporting Pad_cgo* fields on dragonfly/amd64. This also adds some additional RLIMIT_* constants. Updates golang/go#18632 Change-Id: I7969d8ab12c07befd7b1ff0d1dbc67f6622c39b1 Reviewed-on: https://go-review.googlesource.com/100595 Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
|
Change https://golang.org/cl/102075 mentions this issue: |
Re-run ./mkall.sh to avoid exporting Pad_cgo* fields on Darwin. Updates golang/go#18632 Change-Id: Id9d264293d0b1f1c5581aff289c6e826d63e71ef Reviewed-on: https://go-review.googlesource.com/102075 Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Matt Layher <mdlayher@gmail.com>
When the s390x port was upstreamed it was decided that 'to try and stop the bleeding' we would avoid exposing 'internal' fields (usually prefixed with
X__) in structs in the syscall package, instead replacing them with blank identifiers. This work then also made its way into x/sys/unix.Unfortunately every now and again it turns out that someone is using a field we blanked out, for example: #18628 and #17298. This is annoying because while the code in question is probably doing something unpleasant it makes the process of enabling a package on s390x harder than it needs to be.
I would like to propose that we either expose these fields on s390x (only where they exist on other architectures already of course) OR we use blank identifiers for them on other architectures too. Exposing them on s390x is easy, but maybe removing them from the API is more desirable in the long run.
The text was updated successfully, but these errors were encountered: