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/tools/playground: vet step uses different integer sizes than build step #27885

Open
bcmills opened this issue Sep 26, 2018 · 4 comments
Open
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Sep 26, 2018

These two links both emit red errors:
https://play.golang.org/p/b1Bm8V8Eh4S
https://play.golang.org/p/0ZYaKc42JZe

One of them says that the array passed to the function is 16 bytes but the function requires 12, and the other one says that the array is 12 bytes but the function requires 16. The function definition is exactly the same in both cases, so they can't both be correct.

CC @dmitshur @griesemer @bradfitz

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 26, 2018
@bcmills bcmills added this to the Unplanned milestone Sep 26, 2018
@robpike
Copy link
Contributor

robpike commented Sep 26, 2018

That's a good 'un.

@griesemer
Copy link
Contributor

griesemer commented Sep 27, 2018

go vet uses go/types which doesn't use the same sizes as the compiler. Specifically, the go/types.Config struct allows the specification of Sizes. By default it is using amd64 sizing (which still may be slightly different from cmd/compile - e.g., by default go/types produces less internal fragmentation).

The problem here is that the playground uses the 32bit compiler. In the first example, go vet complains because it computes the size of uintptr (8) and int64 (8) as 16, which is not what the array size (12) is. But the compiler produces for the sizes 4 (uintptr) + 8 == 12 and hence the code runs.

In the 2nd example, the array size is 16 which matches what go/types computes, and hence go vet is silent but the compiler complains.

To fix this, the playground would have to use a go/vet version that assumes 32bit word sizes, or the playground could be updated to amd64.

Come to think of if, maybe go/vet needs to have a flag for this as some people might use it with 386-based compiler.

This is mostly a NeedsDecision. The fix is straight-forward.

@griesemer griesemer added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 27, 2018
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Sep 27, 2018
@bradfitz
Copy link
Contributor

The playground is nacl. We'd like to one day move to something else (e.g. gVisor: #25224) but it's not going to happen soon.

In the short term we should teach vet about nacl/amd64p32's weirdo sizes.

@griesemer
Copy link
Contributor

@bradfitz Yes, amd64p32 is what I meant (hence 4 + 8). Teaching vet about this probably requires some flag (if we want to cross-platform check), or alternatively it could just look at GOARCH. Perhaps the latter. @robpike, any opinion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

5 participants