go/types, types2: fix SizesFor("gc", ...) to match actual gc behavior

[griesemer]

Replace https://go.dev/cl/501495 with proper fix in go/types.

[gopherbot]

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

[gopherbot]

Change https://go.dev/cl/506716 mentions this issue: cmd/compile: use types2.Sizes instead of compiler own implementation

[gopherbot]

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

[griesemer]

For future reference: The typechecker's StdSizes documentation explains how sizes are computed. If we want to use it for gc sizes, we need to either update the documentation for array and struct sizes, or add a flag to StdSizes that allows clients to choose the new behavior.

https://go.dev/cl/506856 already has an implementation that uses an internal (const) flag to select the appropriate sizes computation. We could simply move this flag into the StdSizes struct and expose it as a field. This way we don't change the existing behavior for backward-compatibility and also can enable the new behavior as needed.

[rsc]

Kicking over to proposal process for minor API change (even if the only change is in doc comments).

[rsc]

Worth noting that we cannot return a different type from SizesFor, because x/tools/go/package assumes StdSizes:

		// types.SizesFor always returns nil or a *types.StdSizes.
		response.dr.Sizes, _ = sizes.(*types.StdSizes)

[cuonglm]

Worth noting that we cannot return a different type from SizesFor, because x/tools/go/package assumes StdSizes:

		// types.SizesFor always returns nil or a *types.StdSizes.
		response.dr.Sizes, _ = sizes.(*types.StdSizes)

Yeah, I realized this when doing https://go-review.googlesource.com/c/go/+/506856.

[rsc]

The two choices here are:

1. Change the behavior of StdSizes to match the gc and gccgo compilers, assuming they agree.
2. OR, add a new flag field to StdSizes to control which "standard" to use.

It would be highly preferable to do (1), assuming gc and gccgo do agree and go/types is the current outlier.

@ianlancetaylor do you know whether gc and gccgo disagree about any specific type size calculations?

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[ianlancetaylor]

The gofrontend code used by gccgo and GoLLVM lets the backend determine the sizes of all types. The effect is that gccgo and GoLLVM use the C ABI on all platforms. This does indeed mean different type characteristics in some cases, though they are mostly the same.

For example for GOARCH=386 this programs prints 4 with the gc compiler and 8 with the gccgo compiler.

package main

import (
	"fmt"
	"unsafe"
)

func main() {
	fmt.Println(unsafe.Alignof(float64(0)))
}

[cuonglm]

@ianlancetaylor It seems to me that the only difference between gc and gccgo in StdSizes implementation is how word size and max align are setup. For example, this program prints 4 for both gc and gccgo:

package main

import (
	"go/types"
)

func main() {
	println(types.SizesFor("gc", "386").Alignof(types.Typ[types.Float64]))
	println(types.SizesFor("gccgo", "386").Alignof(types.Typ[types.Float64]))
}

So probably we could just go for (1) in @rsc 's comment.

cc @griesemer

[rsc]

It sounds like we cannot make StdSizes return numbers matching gc and gccgo because the two don't match.
So we need some indication in the StdSizes struct for which algorithm we're using.

The constraints are that types.SizesFor returns a *StdSizes, and so it needs to return the StdSizes filled out differently for SizesFor("gc", "386") and SizesFor("gccgo", "386"), but we need to know what extra information to add to StdSizes.

(We'd rather not have 'Compiler string' in StdSizes. It seems better to have the specific details from the algorithm, and let the compiler name be "compiled away" by SizesFor.)

[zephyrtronium]

Worth noting that we cannot return a different type from SizesFor, because x/tools/go/package assumes StdSizes:

		// types.SizesFor always returns nil or a *types.StdSizes.
		response.dr.Sizes, _ = sizes.(*types.StdSizes)

I don't see why this means SizesFor cannot return a different type. SizesFor was never documented to always return a *StdSizes. I would call this x/tools/go/package depending on an implementation detail. Is it really easier to change the definition of StdSizes for everyone than to fix the assumption in x/tools?

[ianlancetaylor]

On linux-ppc64le, this program changes behavior with gc and gccgo.

package main

import (
       "fmt"
       "unsafe"
)

type S struct {
     b byte
     f float64
}

func main() {
	fmt.Println(unsafe.Sizeof(S{}), unsafe.Offsetof(S{}.f))
}

With the gc compiler this prints 16 8, with gccgo with the -malign-power option it prints 12 4. (Tested on a linux-ppc64le system). Note that for gccgo this only applies to fields in structs; the alignment of a standalone float64 variable is 8.

On 32-bit ARM, with the -mabi=aapcs option, gccgo will align struct fields of type int64 or float64 or uint64 to a 64-bit boundary, but gc will align them to a 32-bit boundary. The same is true on 32-bit MIPS by default. (I didn't test this, though.)

I wasn't able to quickly find other examples where gc and gccgo will lay out structs differently.

Since gccgo will change struct field layout depending on command line options, it's more or less impossible to have a fully accurate general implementation of types.Sizes for gccgo. In particular there is no StdSizes value that will work.
People who really care will have to pass in their own implementation of the types.Sizes interface.

Currently in go/types we get slightly different results for arm, mips, and mipsle. For each of those gc uses a MaxAlign of 4 and gccgo uses a MaxAlign of 8. That seems accurate for the default gccgo options.

So if we want to be fully accurate, I think we need to permit users to pass in their own types.Sizes implementation, as the go/types package permits, and we need to change x/tools/go/packages to not assume StdSizes is the only possibility. I don't know how difficult the latter change is. Presumably the type would have to be JSON serializable.

[cuonglm]

Given the fact that StdSizes calculation is already not compatible with gccgo:

types.SizesFor("gccgo", "386").Alignof(types.Typ[types.Float64]) // 4

and:

println(unsafe.Alignof(float64(0))) // print 8 with gccgo

maybe we could just deprecate gccgo argument to SizesFor, and make StdSizes always use gc calculation. Then adding a note that users who want to match gccgo behavior should pass its own types.Sizes implementation like @ianlancetaylor said above.

[gopherbot]

Change https://go.dev/cl/511195 mentions this issue: go/packages: avoid unnecessary dependency on StdSizes

[gopherbot]

Change https://go.dev/cl/519295 mentions this issue: gopls/internal/lsp/cache: avoid dependency on StdSizes

[gopherbot]

Change https://go.dev/cl/520435 mentions this issue: cmd: go get golang.org/x/tools@74c255b and revendor

[gopherbot]

Change https://go.dev/cl/520835 mentions this issue: all: upgrade x/tools and its dependencies

[gopherbot]

Change https://go.dev/cl/520855 mentions this issue: all: upgrade x/tools and its dependencies

[bcmills]

We've seen quite a few tests fail in Go subrepos and in Google-internal testing from this.

Is it feasible to add a GODEBUG guarding the fix, so that modules that declare go versions before 1.22 (and don't override GODEBUG explicitly) can retain the old (buggy) behavior?

[cuonglm]

We've seen quite a few tests fail in Go subrepos and in Google-internal testing from this.

Is it feasible to add a GODEBUG guarding the fix, so that modules that declare go versions before 1.22 (and don't override GODEBUG explicitly) can retain the old (buggy) behavior?

I'm not sure it's worth, given Russ's comment here: #61035 (comment)

I think most of the test failed because it's buggy code which is relying on SizesFor always returns StdSizes, we should rather fix them then leaving them continue with buggy behavior.

[gopherbot]

Change https://go.dev/cl/546837 mentions this issue: doc: add release note for changes to go/types/SizesFor