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

Jump to bottom

go/types: vet and test fail to typecheck valid package using unsafe.Sizeof #60431

Closed
zephyrtronium opened this issue May 25, 2023 · 8 comments
Closed

go/types: vet and test fail to typecheck valid package using unsafe.Sizeof #60431

zephyrtronium opened this issue May 25, 2023 · 8 comments
Assignees
@griesemer
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.22

Comments

@zephyrtronium
Copy link
Contributor

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

$ go version
go version go1.20.4 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
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/branden/.cache/go-build"
GOENV="/home/branden/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/branden/go/pkg/mod"
GONOPROXY="*.example.net"
GONOSUMDB="*.example.net"
GOOS="linux"
GOPATH="/home/branden/go"
GOPRIVATE="*.example.net"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.4"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/branden/work/example/arapi/go.mod"
GOWORK="/home/branden/work/example/go.work"
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2431293305=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Statically asserted the size of a generated struct to ensure that I wouldn't forget to update tests if the definition from which it is generated changes:

package model

type Invoice struct {
	Num       int                  `json:"num"`       // num
	Ot        sql.NullString       `json:"ot"`        // ot
	Oi        sql.NullString       `json:"oi"`        // oi
	Cust      int64                `json:"cust"`      // cust
	Store     sql.NullString       `json:"store"`     // store
	Status    sql.NullString       `json:"status"`    // status
	Afe       sql.NullString       `json:"afe"`       // afe
	Cc        sql.NullString       `json:"cc"`        // cc
	Pono      sql.NullString       `json:"pono"`      // pono
	Invdate   sql.NullTime         `json:"invdate"`   // invdate
	Svcdate   sql.NullTime         `json:"svcdate"`   // svcdate
	Reqdate   sql.NullTime         `json:"reqdate"`   // reqdate
	Shipdate  sql.NullTime         `json:"shipdate"`  // shipdate
	Siteid    NullUniqueidentifier `json:"siteid"`    // siteid
	Site      sql.NullString       `json:"site"`      // site
	Deptid    NullUniqueidentifier `json:"deptid"`    // deptid
	Dept      sql.NullString       `json:"dept"`      // dept
	Pbid      NullUniqueidentifier `json:"pbid"`      // pbid
	Pbno      sql.NullString       `json:"pbno"`      // pbno
	Pb        sql.NullString       `json:"pb"`        // pb
	Shiptoid  sql.NullString       `json:"shiptoid"`  // shiptoid
	Shipto    sql.NullString       `json:"shipto"`    // shipto
	Shipaddr1 sql.NullString       `json:"shipaddr1"` // shipaddr1
	Shipaddr2 sql.NullString       `json:"shipaddr2"` // shipaddr2
	Shipaddr3 sql.NullString       `json:"shipaddr3"` // shipaddr3
	Shipaddr4 sql.NullString       `json:"shipaddr4"` // shipaddr4
	Total     sql.NullFloat64      `json:"total"`     // total
	Subtotal  sql.NullFloat64      `json:"subtotal"`  // subtotal
	Statetax  sql.NullFloat64      `json:"statetax"`  // statetax
	Countytax sql.NullFloat64      `json:"countytax"` // countytax
	Citytax   sql.NullFloat64      `json:"citytax"`   // citytax
	Othertax  sql.NullFloat64      `json:"othertax"`  // othertax
	Pbcheck   sql.NullTime         `json:"pbcheck"`   // pbcheck
	Descr     sql.NullString       `json:"descr"`     // descr
	// xo fields
	_exists, _deleted bool
}
package front

var _ = [1]struct{}{}[unsafe.Sizeof(model.Invoice{})-587]

Then build, vet, and test:

$ go build ./front/
$ go vet ./front/
$ go test ./front/

What did you expect to see?

PASS.

What did you see instead?

  • go build succeeds.
  • go vet reports vet: front/order_test.go:262:23: invalid argument: index 160 out of bounds [0:1].
  • go test reports front/order_test.go:262:23: invalid argument: index 197 out of bounds [0:1].

Likely relates to #40322, in particular #40322 (comment).
@mknyszek mknyszek added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go labels May 30, 2023
@mknyszek mknyszek added this to the Backlog milestone May 30, 2023
@mknyszek mknyszek removed the GoCommand cmd/go label May 30, 2023
@mknyszek mknyszek changed the title cmd/go: vet and test fail to typecheck valid package using unsafe.Sizeof cmd/compile: vet and test fail to typecheck valid package using unsafe.Sizeof May 30, 2023
@mknyszek mknyszek changed the title cmd/compile: vet and test fail to typecheck valid package using unsafe.Sizeof go/types: vet and test fail to typecheck valid package using unsafe.Sizeof May 30, 2023
@mknyszek
Copy link
Contributor

CC @griesemer @findleyr

@griesemer griesemer self-assigned this May 30, 2023
@findleyr
Copy link
Contributor

I'm surprised that we get different results for vet and test; presumably they should be using the same Sizes implementation.

@zephyrtronium
Copy link
Contributor Author

I found some more time to work on a better reproducer and realized that go build ./front/ works because it doesn't include front/order_test.go. Including the size check in a file that is built with the package makes go build produce the same message as go test, because it is an error while compiling the package, while go vet continues to report the same error it had.

The correct constant is 784 rather than 587; using this makes go build succeed again, while go vet and go test then both report constant -37 ... overflows uintptr. Presumably this comes from vet in both invocations.

For the issue with vet, here is a smaller reproducer:

package issue60431

import (
	"unsafe"
)

type Bug struct {
	_ int32
	_ bool
}

var _ = [1]struct{}{}[unsafe.Sizeof(Bug{})-8]

Here go build succeeds while go vet and go test both report unsafe.Sizeof(Bug{}) - 8 (constant -3 of type uintptr) overflows uintptr. In other words, the issue is that go vet's types.Sizes implementation is not including end-of-struct padding to alignment in Sizeof. Considering #40322 (comment), I think this is indeed a duplicate of #40322.

@griesemer
Copy link
Contributor

griesemer commented May 31, 2023 •
edited

The default size computation of go/types doesn't match the compiler, and one of the things that is different is the missing end-of-struct padding, as you have found out (I've argued in the past that that padding can lead to quite a bit of waste if such structs are used as elements in array or slices).

Running the following program in the type checker testing framework (which understands the trace built-in):

package p

import "unsafe"

type S struct {
	_ int32
	_ bool
}

var _ = trace(unsafe.Sizeof(S{}))

prints:

testdata/manual.go:10:15: unsafe.Sizeof(S{}) (constant 5 of type uintptr)

That is, the size of S is 5 bytes as one would expect for a 32bit int and a bool field.

The "correct" fix is for go vet to provide a go/types.Sizes implementation that matches the compiler size computations.

@cuonglm cuonglm mentioned this issue Jun 7, 2023
Closed
@gopherbot
Copy link

Change https://go.dev/cl/501495 mentions this issue: go/analysis: add Sizes that matches size computations

@gopherbot
Copy link

Change https://go.dev/cl/501475 mentions this issue: go/types,types2: fix calc sizeof struct type

@ianlancetaylor ianlancetaylor mentioned this issue Jun 12, 2023
Closed
gopherbot pushed a commit to golang/tools that referenced this issue Jun 27, 2023
@cuonglm @gopherbot
go/analysis: add Sizes that matches gc size computations
5aa6acb 
For golang/go#60431
For golang/go#60734

Change-Id: I6a15a24e3e121635b33d77fde9170a41514c0db1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/501495
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
@gopherbot
Copy link

Change https://go.dev/cl/506715 mentions this issue: go/types, types2: add Sizes that match actual gc behavior

@gopherbot
Copy link

Change https://go.dev/cl/506856 mentions this issue: go/types, types2: add Sizes that match actual gc behavior

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 28, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.22 Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@griesemer griesemer

Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.22
Development

No branches or pull requests
6 participants
@mknyszek @zephyrtronium @dmitshur @griesemer @gopherbot @findleyr