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

cc @cherrymui @randall77

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()
}

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.

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

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.

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.)

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?

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?

@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.

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.

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

@randall77 @cherrymui should we backport this?

@randall77 @cherrymui should we backport this?

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

Reopen for considering backport discussion.

Kindly cc @randall77 @cherrymui

SGTM for backporting.

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

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.

Closing this as it is fixed.