Change https://go.dev/cl/405394 mentions this issue: fix when "x[i] op= y" evaluates x[i] more than once

cc @thanm

I don't think that the language spec provides any guarantees about the order of evaluation of this statement. If you think that it does guarantee the order, can you explain your reasoning? Thanks.

CC @griesemer @mdempsky

I think the issue is valid. Clearer repro:

package main

func main() {
	one := map[int]string{0: "one"}
	two := map[int]string{0: "two"}

	x := one
	x[0] += func() string {
		x = two
		return ""
	}()

	println(one[0], two[0])
}

This program should print one two, but with "gccgo (Debian 11.2.0-16+build1) 11.2.0" it prints one one.

Rationale: the x in x[0] += ... should only be evaluated once. It's unspecified whether x is evaluated before or after the RHS function call, so x may evaluate to either map. But whichever map it evaluates to, we should load from and store back to the same map.

Printing one one demonstrates that gccgo is loading from the first map, and then writing back to the second map. That's incorrect.

I would agree with that. Per the spec

The assignment proceeds in two phases. First, the operands of index expressions and pointer indirections (including implicit pointer indirections in selectors) on the left and the expressions on the right are all evaluated in the usual order. Second, the assignments are carried out in left-to-right order.

So the operand x of x[0], should be evaluated at most once.

OK, thanks.

Change https://go.dev/cl/405616 mentions this issue: test: add test that gofrontend miscompiles

Change https://go.dev/cl/405617 mentions this issue: compiler: load LHS subexpressions of op= assignment only once

We found this issue by testing GoLLVM using the test case:
fixedbugs/issue21687.go

@huanglanzhiguan Thanks. That would have been a helpful detail to mention in the original issue report.

I've written a patch for the gofrontend to attempt to fix this problem (https://go.dev/cl/405617). When I run the initial program in this issue with the gc compiler, it prints 3501. When I run it with the patched gofrontend, I get 2388.

This hinges on how we execute

    x[0] += func() int {
        x = two
        return 1113
    }()

I'm currently executing it as

    t1 := x
    t2 := 0
    t3 := func() int { x = two; return 1113 }()
    t1[t2] = t3

When I do that, the program prints 2388.

The only way to get 3501 is for the function to be executed before loading x. I don't see why that is mandatory.

Does anybody think that the gofrontend is incorrect in printing 2388? Does anybody think that gc is incorrect in printing 3501? Thanks.

It's correct for the reported, obfuscated test case to print either 2388 or 3501 (2388+1113). It's just incorrect to print other values, including 2412 (1299+1113).

The original issue21687.go test case was written specifically to be flexible about order-of-evaluation variations in compilers.

The new gofrontend behavior sounds correct to me.