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: incorrect value for TCGETS and TCSETS in std syscall pkg on ppc64x #19560

Closed
laboger opened this issue Mar 15, 2017 · 13 comments
Closed

syscall: incorrect value for TCGETS and TCSETS in std syscall pkg on ppc64x #19560

laboger opened this issue Mar 15, 2017 · 13 comments
Milestone

Comments

@laboger
Copy link
Contributor

@laboger laboger commented Mar 15, 2017

Please answer these questions before submitting your issue. Thanks!

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

go 1.8, master

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

Ubuntu and RH ppc64le and ppc64

What did you do?

Tried to use ioctl syscall with TCGETS on ppc64le.

What did you expect to see?

Valid result.

What did you see instead?

Error message: inappropriate ioctl for device

This error occurs when using the syscall package but works when using golang.org/x/sys/unix due to the fact that these two packages define different values for TCGETS and TCSETS:

In golang syscall package, ztypes_linux_ppc64le.go and ztypes_linux_ppc64.go:

    TCGETS = 0x403c7413
    TCSETS = 0x803c7414

In golang.org/sys/unix/zerrors_linux_ppc64le.go and zerrors_linux_ppc64.go

TCGETS = 0x402c7413
TCSETS = 0x802c7414

I see there is a comment in the golang.org documentation about the use of the syscall package being "locked down". My assumption is that the syscall package should be fixed since it is now incorrect.

Testcase:
ioctlbug.go.txt

Failing execution:
Error on get: inappropriate ioctl for device

importing from golang.org/x/sys/unix allows the test to work.

@laboger laboger changed the title syscall: incorrect value for TCGETS and TCSETS in std syscall pkg on ppc64lx syscall: incorrect value for TCGETS and TCSETS in std syscall pkg on ppc64x Mar 15, 2017
@clnperez
Copy link

@clnperez clnperez commented Mar 15, 2017

This was found as a result of investigating opencontainers/runc#1364

@laboger
Copy link
Contributor Author

@laboger laboger commented Mar 16, 2017

@bradfitz Who can answer questions about this issue?

Is the answer supposed to be that users should be using golang.org/x/sys/unix instead of syscall from golang std?

Personally I think that the syscall package in golang should be correct. But it is not clear to me how to get the correct value generated from the sh and pl files that are used to create them, and why the one in golang.org/x is correct but the other is not.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 16, 2017

Why is std incorrect? Have these values changed over time? If so, that suggests even more that the answer is using x/sys/unix and not trying to keep syscall up to date.

But yes, the real answer is that everybody should use x/sys and pretend that the std package "syscall" doesn't exist.

@bradfitz bradfitz added this to the Unplanned milestone Mar 16, 2017
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 16, 2017

I'll slightly disagree with @bradfitz and say that if the values in the syscall package were always wrong, then I think it would be OK to correct them. It looks like they were introduced by @minux in https://golang.org/cl/127170043, where he says they were autogenerated. Minux, do you know anything about these values?

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 16, 2017

Yeah, if they were always wrong and they by definition never change and it's just a result of a bug in our generator program, I'm fine changing it. But that does mean that users will need to wait for a new version of Go for the fix, whereas if runc used golang.org/x/sys/unix, they'd have the fix already.

@laboger
Copy link
Contributor Author

@laboger laboger commented Mar 17, 2017

It looks like this problem was identified here https://groups.google.com/forum/#!msg/golang-nuts/K5NoG8slez0/mixUse17iaMJ and fixed some time after that in golang.org/x/sys/unix.

This explains why syscall and golang.org/x/sys/unix are different. And just fixing one value in syscall might not be enough -- when other ioctl functions are used they could be wrong also.

@clnperez
Copy link

@clnperez clnperez commented Mar 17, 2017

So it looks like they were always wrong, and it would be good to fix this since so many projects do still syscall. I see that the rec is definitely to move (https://groups.google.com/forum/#!topic/golang-dev/cEASnHIXmLI), but at least the container-focused projects haven't yet.

@bradfitz, I did a quick substitution of syscall for sys in parts of the runc code, and it looks like there are some things missing from sys like Signal. Actually, it looks like syscall.Signal is used by sys, so, there's no clean break from syscall. Is this something I should take up on the mailing list if I want to get advice on moving over? It seems strange to mix the two.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 17, 2017

I can't think of a reason for golang.org/x/sys/unix to use syscall.Signal. Perhaps we should just replace it. Anyhow, yes, raise this on golang-dev. Thanks.

@laboger
Copy link
Contributor Author

@laboger laboger commented Mar 24, 2017

I've been trying to understand how sys/unix could be fixed, but I can't get the generator scripts to work. Here are the errors I get when I run this:
GOOS=linux GOARCH=ppc64le ./mkall.sh
could not determine kind of name for C.TIOCGETC
could not determine kind of name for C.TIOCGETP
could not determine kind of name for C.TIOCGLTC
could not determine kind of name for C.TIOCSETC
could not determine kind of name for C.TIOCSETN
could not determine kind of name for C.TIOCSETP
could not determine kind of name for C.TIOCSLTC

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 24, 2017

What do the definitions for those symbols look like in the header files on your system?

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 4, 2017

CL https://golang.org/cl/39450 mentions this issue.

@laboger
Copy link
Contributor Author

@laboger laboger commented Apr 4, 2017

@ianlancetaylor Sorry somehow I missed your previous question.

The errors above occur because these values are defined in terms of the _IOR macro and the set of header files as they are now don't provide the definition of that before it is used AFAICT.

What we have discovered is that on some platforms these are defined in asm/termios.h but on others it is in asm/termbits.h. So when files are changed to use the right header files for ppc64x, that results in many other changes in the zppc64.go files as well as other platforms I'm sure.

Something like this seems to make it work by changing mkerrors.sh, as well as a change with the same effect in linux_types.go:
//+if [[ "$GOARCH" == "ppc64" || "$GOARCH" == "ppc64le" ]]; then
//+ includes_Linux+='
//+#include <asm/termios.h>
//+'
//+else
//+ includes_Linux+='
//+#include <sys/ioctl.h>
//+#include <sys/mount.h>
//+#include <asm/termbits.h>
//+'
//+fi
//+

@gopherbot gopherbot closed this in ed2f913 Apr 5, 2017
lparth added a commit to lparth/go that referenced this issue Apr 13, 2017
The comments in this package state that users should be
migrating code that uses the syscall package to its
corresponding package in x/sys. However, the syscall.Signal
and syscall.Errno types and the syscall.SysProcAttr struct is
not defined in the x/sys package and still need to be referenced
from within syscall.  This adds a change to the comments to
clarify that the migration will need to continue to use some
references to syscall for now.

Fixes golang#19560

Change-Id: I8abb96b93bea90070ce461da16dc7bcf7b4b29c1
Reviewed-on: https://go-review.googlesource.com/39450
Reviewed-by: Rob Pike <r@golang.org>
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 27, 2017

Change https://golang.org/cl/66510 mentions this issue: syscall: correct TCGETS/TCSETS values on ppc64/ppc64le

gopherbot pushed a commit that referenced this issue Sep 27, 2017
Correcting values is allowed per the syscall package rules, so update
these constants to their correct value on ppc64/ppc64le. The values now
match the corresponding constants in x/sys/unix.

Update #19560
Fixes #22000

Change-Id: I1d358de345766ec96e15dfcc8911fe2f39fb0ddb
Reviewed-on: https://go-review.googlesource.com/66510
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Lynn Boger <laboger@linux.vnet.ibm.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Sep 27, 2018
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.