cmd/compile: internal compiler error: 'F': func F, startMem[b1] has different values

[ALTree]

$ go version
go version go1.21.0 linux/amd64

$ gotip version
go version devel go1.22-ac64a362 Sat Aug 12 03:56:58 2023 +0000 linux/amd64

package p

type S1 struct {
	a, b, c []int
	i       int
}

type S2 struct {
	a, b []int
	m    map[int]int
}

func F(i int, f func(S1, S2, int) int) int {
	return f(
		S1{},
		S2{m: map[int]int{}},
		1<<i)
}

$ gotip build crash.go 

# command-line-arguments
./crash.go:17:3: internal compiler error: 'F': func F, startMem[b1] has different values, old v1, new v29

goroutine 9 [running]:
runtime/debug.Stack()
	./desktop/gotip/src/runtime/debug/stack.go:24 +0x5e
cmd/compile/internal/base.FatalfAt({0x41f678?, 0xc0?}, {0xc00002bf00, 0x40}, {0xc00040ee60, 0x5, 0x5})
	./desktop/gotip/src/cmd/compile/internal/base/print.go:230 +0x1d7
cmd/compile/internal/base.Fatalf(...)
	./desktop/gotip/src/cmd/compile/internal/base/print.go:199
cmd/compile/internal/ssagen.(*ssafn).Fatalf(0x10?, {0x589a2108?, 0x7eff?}, {0xd978d9, 0x3a}, {0xc000405c00, 0x4, 0x0?})
	./desktop/gotip/src/cmd/compile/internal/ssagen/ssa.go:8017 +0x16a
cmd/compile/internal/ssa.(*Func).Fatalf(0xc00037ba00, {0xd978d9, 0x3a}, {0xc000405c00, 0x4, 0x4})
	./desktop/gotip/src/cmd/compile/internal/ssa/func.go:716 +0x279
cmd/compile/internal/ssa.memState(0xc00037ba00, {0xc000022800, 0x4, 0x0?}, {0xc000022b00, 0x4, 0x0?})
	./desktop/gotip/src/cmd/compile/internal/ssa/tighten.go:240 +0x62a
cmd/compile/internal/ssa.tighten(0xc00037ba00)
	./desktop/gotip/src/cmd/compile/internal/ssa/tighten.go:30 +0x228
....

Doesn't crash on go1.20.7

cc @golang/compiler

[gopherbot]

Change https://go.dev/cl/519115 mentions this issue: Hotfix/61992 left shift operator call go1.21

[cuonglm]

git bisect points to https://go-review.googlesource.com/c/go/+/492895

cc @erifan @randall77

[randall77]

This looks like a bug in expand calls. It is confused about which memory to use and gets the memory chain in a bad state. The CL mentioned just notices that bug.

[dr2chase]

This works fine in the cleaned-up expand calls. https://go.dev/cl/507295

[randall77]

That's good for tip, but what can we reasonably backport to 1.21?
Maybe just dropping that check is reasonable, as this "bug" has been around for a while. (Just on the 1.21 branch.)

[dr2chase]

I've been poking at the old code, and it is horrible, and the expand calls cleanup cherry-picks cleanly onto 1.21.
I'm about to test it but given when I wrote the cleanup, it is not a surprise if it works.

[erifan]

I'm wondering if it's more reasonable to backport a fix for the expand calls pass, because this check is correct and valuable, and it has also checked out another bug before, see #59973.
Check the IR graph, it looks like moving v29 and v30 to b1 will solve the problem.

[dr2chase]

The rough cause is that arguments are marshalled "blindly" in argument order, and those that must go on stack are stored, which affects memory.

If args (or results) occur in a different blocks that constrains their order; so would a data flow between two of the arguments (e.g., "P" and "pointerToP"). I think a better "fix" is to (1) iterate in block order (2) do all the not-memory args first (3) record memory replacements in a map. I'm a little worried about complex control leading to calls (and Phi functions), but I think that data flow dependences are okay because pointers args require a load-then-store, and the loads will just float early (not be threaded onto the new mem).

This would also require additional load-updating in mem uses in subsequent blocks. I think this will (usually?) be limited to the call.

This should also cause problems with return values. The refactored code might still have this problem (potentially) or it may dodge it by handing the memory re-threading to the rewrite cleanup pass; if it does have this problem for other inputs, it is less complicated and easier to understand.

[dr2chase]

Bonus fun, it's a closure call, by default the first parameter is ignored by expand calls but in this case it is a load and thus retains a pointer to the "old" memory.

[randall77]

I'm not sure I understand why we need to put the arg marshaling anywhere but the block of the call itself.
The code for rewriteDereference, say, uses the block of the dereference, but it could just as easily use the call block, because before calling rewriteDereference we check that the memory arguments of the call and dereference are the same.

[dr2chase]

The extra block appears because of a check for an illegal shift value. The minimum-touch fixes moves the dereference rewrite to always in the block containing the dereference (in bug-world, it depends on the size of the dereference) and also does an update to the mem of the closure parameter. That is my current understanding, at least.

[dr2chase]

I tried moving the arg marshalling into the call block (it was not there) and that seems to work, and that is probably the best fix. That also looks like what the rewritten expand_calls does. I am however now a little curious about one of the changes that I tried, which was to do all the memory args first, under the assumption that having more registers free to accomplish memory movement is a good thing. But that is not this bug.

[gopherbot]

Change https://go.dev/cl/519276 mentions this issue: cmd/compile: in expandCalls, move all arg marshalling into call block

[dr2chase]

@gopherbot please open the backport tracking issues. This is a use-blocking compiler bug.

[gopherbot]

Backport issue(s) opened: #62056 (for 1.20), #62057 (for 1.21).

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

[randall77]

@Nasfame It is a good question whether we need a backport to 1.20. It is not needed for this issue, but we probably want to fix the store chain even if there's no subsequent pass that notices. Because maybe there is a pass that notices, silently generating incorrect code.
I could be convinced otherwise if there was an argument that the broken memory chain can't cause an issue. But without such an argument I'm sufficiently nervous about it to warrant a backport.

[gopherbot]

Change https://go.dev/cl/520058 mentions this issue: [release-branch.go1.21] cmd/compile: in expandCalls, move all arg marshalling into call block

[gopherbot]

Change https://go.dev/cl/520059 mentions this issue: [release-branch.go1.20] cmd/compile: in expandCalls, move all arg marshalling into call block

[cagedmantis]

@randall77 @dr2chase There are currently backport issues for both 1.20 and 1.21 which will be approved pending the decision on whether we need a backport for both 1.20 and 1.21. Do we need both backports or just a single backport?

[randall77]

I think we need to backport to both 1.21 and 1.20, see my comment above.