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?

[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