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.

Sign up for GitHub

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

Jump to bottom

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

Closed
ALTree opened this issue Aug 13, 2023 · 26 comments
Closed

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

ALTree opened this issue Aug 13, 2023 · 26 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
Go1.22

Comments

@ALTree
Copy link
Member

ALTree commented Aug 13, 2023 

$ 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
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 13, 2023
@ALTree ALTree added this to the Go1.22 milestone Aug 13, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 13, 2023
@gopherbot
Copy link

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

@cuonglm
Copy link
Member

cuonglm commented Aug 14, 2023

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

cc @erifan @randall77

@randall77
Copy link
Contributor

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
Copy link
Contributor

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

@randall77
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

erifan commented Aug 15, 2023 •
edited

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

@dr2chase
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

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

@dr2chase
Copy link
Contributor

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

This was referenced Aug 15, 2023
Closed
Closed
@gopherbot
Copy link

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
Copy link
Contributor

@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
Copy link

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
Copy link

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

@dmitshur dmitshur 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 Aug 17, 2023
@cagedmantis
Copy link
Contributor

@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
Copy link
Contributor

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

Pro7ech added a commit to tuneinsight/lattigo that referenced this issue Aug 24, 2023
@Pro7ech
temporary fix for v1.21: golang/go#61992
98b412d
gopherbot pushed a commit that referenced this issue Aug 24, 2023
@dr2chase @cagedmantis
[release-branch.go1.21] cmd/compile: in expandCalls, move all arg mar…
2b8026f 
…shalling into call block

For aggregate-typed arguments passed to a call, expandCalls
decomposed them into parts in the same block where the value
was created.  This is not necessarily the call block, and in
the case where stores are involved, can change the memory
leaving that block, and getting that right is problematic.

Instead, do all the expanding in the same block as the call,
which avoids the problems of (1) not being able to reorder
loads/stores across a block boundary to conform to memory
order and (2) (incorrectly, not) exposing the new memory to
consumers in other blocks.  Putting it all in the same block
as the call allows reordering, and the call creates its own
new memory (which is already dealt with correctly).

Fixes #62057.
Updates #61992.

Change-Id: Icc7918f0d2dd3c480cc7f496cdcd78edeca7f297
Reviewed-on: https://go-review.googlesource.com/c/go/+/519276
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit e72ecc6)
Reviewed-on: https://go-review.googlesource.com/c/go/+/520058
gopherbot pushed a commit that referenced this issue Aug 24, 2023
@dr2chase @cagedmantis
[release-branch.go1.20] cmd/compile: in expandCalls, move all arg mar…
612da32 
…shalling into call block

For aggregate-typed arguments passed to a call, expandCalls
decomposed them into parts in the same block where the value
was created.  This is not necessarily the call block, and in
the case where stores are involved, can change the memory
leaving that block, and getting that right is problematic.

Instead, do all the expanding in the same block as the call,
which avoids the problems of (1) not being able to reorder
loads/stores across a block boundary to conform to memory
order and (2) (incorrectly, not) exposing the new memory to
consumers in other blocks.  Putting it all in the same block
as the call allows reordering, and the call creates its own
new memory (which is already dealt with correctly).

Fixes #62056.
Updates #61992.

Change-Id: Icc7918f0d2dd3c480cc7f496cdcd78edeca7f297
Reviewed-on: https://go-review.googlesource.com/c/go/+/519276
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit e72ecc6)
Reviewed-on: https://go-review.googlesource.com/c/go/+/520059
Pro7ech added a commit to tuneinsight/lattigo that referenced this issue Sep 8, 2023
@Pro7ech
reverted temporary fix for v1.21: golang/go#61992
b413eff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
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
Milestone
Go1.22
Development

No branches or pull requests
9 participants
@cagedmantis @dmitshur @dr2chase @ALTree @cuonglm @randall77 @gopherbot @erifan and others