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

cmd/compile: typebits.Set: invalid initial alignment: type Peer has alignment 8, but offset is 4 #54991

Closed
stv0g opened this issue Sep 10, 2022 · 18 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@stv0g
Copy link

stv0g commented Sep 10, 2022

This is seems to be a regression of: #54638

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

go version go1.19.1 linux/arm64

Does this issue reproduce with the latest release?

Not, tested.

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

go env Output
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/home/stv0g/.cache/go-build"
GOENV="/home/stv0g/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/stv0g/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/stv0g/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_arm64"
GOVCS=""
GOVERSION="go1.19.1"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/stv0g/workspace/cunicu2/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3584615060=/tmp/go-build -gno-record-gcc-switches"

What did you do?

git clone github.com/stv0g/cunicu
cd cunicu
GOARCH=arm go build ./cmd/cunicu

What did you expect to see?

A successful build for 32-bit architectures.

What did you see instead?

The following compiler error:

# github.com/stv0g/cunicu/pkg/feat/epdisc
<autogenerated>:1: internal compiler error: typebits.Set: invalid initial alignment: type Peer has alignment 8, but offset is 4

Please file a bug report including a short program that triggers the error.
https://go.dev/issue/new

This issue has been observed for the following GOARCH=arm, mips and GOOS=linux
64-bit builds (GOOS=arm64, amd64) are successful.

@cuonglm
Copy link
Member

cuonglm commented Sep 10, 2022

It seems to me that CL 425256 only handles local variables. While the problem in this issue is for arguments which are on stack.

@cuonglm cuonglm changed the title internal compiler error: typebits.Set: invalid initial alignment: type Peer has alignment 8, but offset is 4 cmd/compile: typebits.Set: invalid initial alignment: type Peer has alignment 8, but offset is 4 Sep 10, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 10, 2022
@cuonglm cuonglm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 10, 2022
@cuonglm
Copy link
Member

cuonglm commented Sep 10, 2022

cc @cherrymui @randall77

@cuonglm
Copy link
Member

cuonglm commented Sep 12, 2022

Minimal reproducer:

package p

import (
	"sync/atomic"
)

type I interface {
	M()
}

type S struct{}

func (S) M() {}

type T struct {
	I
	x atomic.Int64
}

func F() {
	t := T{I: S{}}
	t.M()
}

@cuonglm cuonglm 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 Sep 12, 2022
@cuonglm cuonglm added this to the Go1.20 milestone Sep 12, 2022
@cuonglm
Copy link
Member

cuonglm commented Sep 13, 2022

Seems that we can't do the same trick with autotmp, the arguments frame offset is used to write function stack map, so it can't be fake.

@cherrymui
Copy link
Member

cherrymui commented Sep 13, 2022

I haven't looked into this issue. But I think CL 425256 should handle autotmp. I'll see what we missed.

@cuonglm
Copy link
Member

cuonglm commented Sep 13, 2022

I haven't looked into this issue. But I think CL 425256 should handle autotmp. I'll see what we missed.

Yeah, see my comment above.

This issue is about parameters which are on stack, specifically for the reproducer, it's .this receiver of wrapper method.

@randall77
Copy link
Contributor

randall77 commented Sep 15, 2022

Seems to be ok for 386, just not arm.
Looks like this is due to the fact that args start at offset 4 from SP. I'm not sure there's anything we can do here, we'd have to change the calling convention which would break existing arm assembly.
We could ignore this error in cases like this, I guess.
Or copy the arg off into an argtmp that is properly aligned in the prolog.
In any case it seems a very strange case. I don't see any reason to pass an atomic thing around as an argument.
Maybe the error could be better though? (Or as above, just make this not an error.)

@cuonglm
Copy link
Member

cuonglm commented Sep 15, 2022

In any case it seems a very strange case. I don't see any reason to pass an atomic thing around as an argument.

To be clear, it's not passing an atomic around, it's the .this receiver, which contains atomic thing:

  • T has alignment 8
  • arm has fixed frame size 4
  • When calculating offset for .this receiver, which has type T, we use its offset minus fixed frame size, which is 8 - 4

Previously, this can't happen because T would have alignment 4, thus frame offset is 4 - 4, which will satisfy the check.

Maybe for typesbits.Set, we should skip the check if T alignment is greater than ptr size?

@stv0g
Copy link
Author

stv0g commented Sep 15, 2022

Hi there 👋,

is there a chance to work around this bug in my current code?

In any case it seems a very strange case. I don't see any reason to pass an atomic thing around as an argument.

This hints, that passing it around as a pointer would work?

@cuonglm
Copy link
Member

cuonglm commented Sep 15, 2022

@stv0g one way to workaround is not using the embedded interface type, making it an explicit field instead. For example, for the reproducer above, you can do:

package p

import "sync/atomic"

type I interface {
	M()
}

type S struct{}

func (S) M() {}

type T struct {
	i I
	x atomic.Int64
}

func F() {
	t := T{i: S{}}
	t.i.M()
}

Notice I is changed from embedded field to i I. That way, the compiler won't have to generate the wrapper.

@randall77
Copy link
Contributor

randall77 commented Sep 15, 2022

Maybe for typesbits.Set, we should skip the check if T alignment is greater than ptr size?

We don't want to do that always, as we do want to be able to align to 8 bytes on 4 byte platforms on the heap. But maybe for all arg/result bitmap generation, yes.

It is all kinda hacky however we handle it.

I was thinking that it wouldn't actually break the calling convention to align things more, as nothing is 8-byte aligned at the moment on arm. So we could pad somehow if we knew one of the args needed >4 byte alignment. But this is going to be a really rare case which makes it hard to ensure that we've got it right. I'm leaning towards just removing the check.

@gopherbot
Copy link

gopherbot commented Sep 16, 2022

Change https://go.dev/cl/431098 mentions this issue: cmd/compile/internal/typebits: relax alignment check

@cuonglm
Copy link
Member

cuonglm commented Sep 17, 2022

@randall77 @cherrymui should we backport this?

@stv0g
Copy link
Author

stv0g commented Sep 19, 2022

@randall77 @cherrymui should we backport this?

I would highly appreciate it as it breaks valid Go code in the current release.

@cuonglm
Copy link
Member

cuonglm commented Sep 19, 2022

Reopen for considering backport discussion.

Kindly cc @randall77 @cherrymui

@cuonglm cuonglm reopened this Sep 19, 2022
@cherrymui
Copy link
Member

cherrymui commented Sep 19, 2022

SGTM for backporting.

@gopherbot please backport this to Go 1.19 . This causes valid program fail to compile.

@gopherbot
Copy link

gopherbot commented Sep 19, 2022

Backport issue(s) opened: #55152 (for 1.19).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@cherrymui
Copy link
Member

cherrymui commented Sep 19, 2022

Closing this as it is fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants