x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment incorrect size calculations, false positives

[iamtakingiteasy]

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

$ go version
go version go1.15.8 linux/amd64

Does this issue reproduce with the latest release?

Reproduces from upstream tools repository

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/go/pkg/mod"
GOOS="linux"
GOPATH="/home/go"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build474839094=/tmp/go-build -gno-record-gcc-switches"

What did you do?

running

./fieldalignment /tmp/z/foo.go on

package main

import (
	"fmt"
	"unsafe"
)

// (1)
type ex1A struct {
	S string
	F int32
}

type ex1B struct {
	F int32
	S string
}

// (2)
type ex2A struct {
	S string
	F *int32
}

type ex2B struct {
	F *int32
	S string
}

func main() {
	var (
		a1 ex1A
		b1 ex1B
		a2 ex2A
		b2 ex2B
		c  int32
		d  string
		e  *int32
	)

	fmt.Printf("%10s : %d\n", "a1", unsafe.Sizeof(a1))       // 24
	fmt.Printf("%10s : %d\n", "b1", unsafe.Sizeof(b1))       // 24
	fmt.Printf("%10s : %d\n", "a2", unsafe.Sizeof(a2))       // 24
	fmt.Printf("%10s : %d\n", "b2", unsafe.Sizeof(b2))       // 24
	fmt.Printf("%10s : %d\n", "int32, c", unsafe.Sizeof(c))  // 4
	fmt.Printf("%10s : %d\n", "string, d", unsafe.Sizeof(d)) // 16
	fmt.Printf("%10s : %d\n", "*int32, e", unsafe.Sizeof(e)) // 8
}

What did you expect to see?

(1) Since string alone is 16 bytes long, and int32 is 4 bytes long, report should read "struct with 24 pointer bytes could be 20" (16+4+4 | 16+4)
Maybe it also superfluous to report structures that cannot be made any smaller such as in this case, since 20 would still be aligned to 24. At very least this should be indicated in report.

(2) No error reported. Since string alone is 16 bytes long and *int32 is 8 bytes long, it would be 16+8 = 24 regardless of position of F.

What did you see instead?

/tmp/z/foo.go:14:11: struct with 16 pointer bytes could be 8
/tmp/z/foo.go:20:11: struct with 24 pointer bytes could be 16

[stamblerre]

/cc @leitzler

[leitzler]

I'll defer to @mdempsky who knows the calculations way better than I do :)

[mdempsky]

Note that diagnostic is about pointer bytes, not about size. That is, how many bytes of the object that the GC has to potentially scan for pointers.

struct { uint32; string } and struct { *uint32; string } have 16 pointer bytes because the GC has to scan up through the string's inner pointer.

struct { string; *uint32 } has 24 pointer bytes because it has to scan further through the *uint32.

struct { string; uint32 } has 8 because it can stop immediately after the string pointer.

That said, this is a pretty niche optimization opportunity, and probably not worth reporting to users by default. While shrinking size may be beneficial, it's likely to be small and there are a lot more tradeoffs here to consider.

[iamtakingiteasy]

Interesting.

At least in case of golangci-lint, which had pretty old maligned version (from 2018), it was pretty unexpected to encounter such new diagnostic when trying to use fieldalignment as a drop-in replacement to maligned linter. But this seem to be an issue for golangci-lint, since nothing is wrong with fieldalignment and it does not break expectations set from modern maligned.

Nevertheless it is an interesting diagnostic and probably should be better documented, as current documentation does not suggest any diagnostic other than memory size.