cmd/compile: inconsistent behaviour in multiple assignment #23017

zigo101 · 2017-12-06T15:34:43Z

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

go version go1.9.2 linux/amd64

Does this issue reproduce with the latest release?

yes

What did you do?

Two programs:

program 1:

package main

import "fmt"

func main() {
  var m = map[int]int{}
  var p *int
  
  defer func() {
    fmt.Println(recover())
    fmt.Println(len(m)) // 0
  }()
  m[2], *p = 1, 2
}

program 2:

package main

import "fmt"

func main() {
  var m map[int]int
  var a int
  var p = &a
  
  defer func() {
    fmt.Println(recover())
    fmt.Println(*p) // 5
  }()
  *p, m[2] = 5, 2
}

What did you expect to see?

The behaviors of the two programs should be consistent.

What did you see instead?

The behaviors of the two programs are not consistent.

program 1 finishes zero assignments.
but program 2 finishes one assignment.

The text was updated successfully, but these errors were encountered:

ianlancetaylor · 2017-12-06T15:41:05Z

The order of evaluation rules for the language do not specify precisely when a pointer indirection occurs. I think that either behavior is permitted by the language.

CC @griesemer

randall77 · 2017-12-06T15:47:14Z

From the spec:

x := []int{1, 2, 3}
type Point struct { x, y int }
var p *Point
x[2], p.x = 6, 7  // set x[2] = 6, then panic setting p.x = 7

That sounds like program 1, we should set m[2]=1 before panicking.

griesemer · 2017-12-06T19:24:22Z

[edited: The conclusion below is wrong, see next comment]

@randall77 I think this example in the spec is wrong. The prose before says:

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.

That is, we shouldn't get to the assignment if there's a nil-pointer exception. I'd conclude the behavior for program 1 is correct.

Secondly, I don't see anything in the spec stating when panics due to assignments to nil maps should happen in multiple assignments; I'd conclude that the behavior for program 2 is at least not incorrect.

We should a) fix the spec example, and b) decide if we want to say anything more about map assignments.

griesemer · 2017-12-06T19:30:37Z

Never mind; I misread. The operands are evaluated, but not the entire expression. So the spec example is correct, and program 1 should assign to m.

mdempsky · 2017-12-06T19:32:02Z

My understanding of the spec is that m[2], *p = 1, 2 should be evaluated as:

// 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.
t0, t1, t2, t3, t4 := m, 2, p, 1, 2

// Second, the assignments are carried out in left-to-right order.
t0[t1] = t3
*t2 = t4

That is, program 1 should print 1.

mdempsky · 2017-12-06T19:36:01Z

Related discussion: #15620.

mdempsky · 2017-12-06T20:51:32Z

Program 1 is miscompiled by cmd/compile because order.go rewrites

m[2], *p = 1, 2

into

t0, t1 := m, 2
t2, *p = 1, 2
t0[t1] = t2

mdempsky · 2017-12-06T21:09:59Z

gccgo prints 1 and 5 for programs 1 and 2, respectively.

randall77 · 2017-12-11T22:43:09Z

Right, the spec says:

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 I take that to mean that in the first phase, we evaluate the right hand side, plus any arguments of * or [] (or . that really mean C's ->) on the left-hand side, but not the * or [] themselves. Then we do the assignments left-to-right. Both of OP's programs panic in phase 2, as the dereference on the left-hand side doesn't happen in phase 1.

See #22881 also.

randall77 · 2017-12-12T00:24:36Z

OP's program 1 fails for all of go1.3-tip. Go 1.2 gets it right!
1.11 sounds correct.

randall77 · 2019-05-06T19:25:47Z

Punting again, too late for 1.13.

cuonglm · 2019-10-04T05:36:13Z

How about:

        var p *int
        m := map[int]int{}
        m[0], _ = append(m[0], 0), *p

Must m[0] modified?

I expected it does, but current test case indicate it is not modified.

cuonglm · 2019-10-04T05:50:44Z

How about:
        var p *int
        m := map[int]int{}
        m[0], _ = append(m[0], 0), *p
Must m[0] modified?

I expected it does, but current test case indicate it is not modified.

Ah ok, it should not modify map, because we evaluate RHS first, but RHS panics.

gopherbot · 2019-10-04T17:06:44Z

Change https://golang.org/cl/198679 mentions this issue: cmd/compile: fix wrong order in multiple assignments contains map and deref

gopherbot · 2019-10-11T19:59:08Z

Change https://golang.org/cl/200580 mentions this issue: cmd/compile: simplify OAS2XXX nodes handle in order

Passes toolstash-check. Updates #23017 Change-Id: I0ae82e28a6e9e732ba2a6aa98f9b35551efcea10 Reviewed-on: https://go-review.googlesource.com/c/go/+/200580 Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>

gopherbot · 2020-10-12T21:32:25Z

Change https://golang.org/cl/261677 mentions this issue: cmd/compile: fix and improve alias detection

"aliased" is the function responsible for detecting whether we can turn "a, b = x, y" into just "a = x; b = y", or we need to pre-compute y and save it in a temporary variable because it might depend on a. It currently has two issues: 1. It suboptimally treats assignments to blank as writes to heap memory. Users generally won't write "_, b = x, y" directly, but it comes up a lot in generated code within the compiler. This CL changes it to ignore blank assignments. 2. When deciding whether the assigned variable might be referenced by pointers, it mistakenly checks Class() and Name.Addrtaken() on "n" (the *value* expression being assigned) rather than "a" (the destination expression). It doesn't appear to result in correctness issues (i.e., incorrectly reporting no aliasing when there is potential aliasing), due to all the (overly conservative) rewrite passes before code reaches here. But it generates unnecessary code and could have correctness issues if we improve those other passes to be more aggressive. This CL fixes the misuse of "n" for "a" by renaming the variables to "r" and "l", respectively, to make their meaning clearer. Improving these two cases shaves 4.6kB of text from cmd/go, and 93kB from k8s.io/kubernetes/cmd/kubelet: text data bss dec hex filename 9732136 290072 231552 10253760 9c75c0 go.before 9727542 290072 231552 10249166 9c63ce go.after 97977637 1007051 301344 99286032 5eafc10 kubelet.before 97884549 1007051 301344 99192944 5e99070 kubelet.after While here, this CL also collapses "memwrite" and "varwrite" into a single variable. Logically, they're detecting the same thing: are we assigning to a memory location that a pointer might alias. There's no need for two variables. Updates #6853. Updates #23017. Change-Id: I5a307b8e20bcd2196e85c55eb025d3f01e303008 Reviewed-on: https://go-review.googlesource.com/c/go/+/261677 Trust: Matthew Dempsky <mdempsky@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>

gopherbot · 2021-01-03T08:38:36Z

Change https://golang.org/cl/281172 mentions this issue: [dev.regabi] cmd/compile: fix map assignment order

ianlancetaylor added this to the Unplanned milestone Dec 6, 2017

ianlancetaylor added the Documentation label Dec 6, 2017

griesemer added NeedsFix and removed Documentation labels Dec 6, 2017

griesemer modified the milestones: Unplanned, Go1.11 Dec 6, 2017

randall77 self-assigned this Dec 12, 2017

mdempsky mentioned this issue Jan 29, 2018

gccgo: incorrect order of evaluation according to spec #23188

Closed

ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 30, 2018

andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019

randall77 modified the milestones: Go1.13, Go1.14 May 6, 2019

rsc modified the milestones: Go1.14, Backlog Oct 9, 2019

gopherbot closed this as completed in 907a4bf Feb 16, 2021

nevkontakte mentioned this issue Sep 17, 2021

Incorrect lhs evaluation order in multi-assignments gopherjs/gopherjs#1063

Closed

nevkontakte mentioned this issue Oct 27, 2021

Incorrect lhs evaluation order in multi-assignments gopherjs/gopherjs#358

Open

golang locked and limited conversation to collaborators Feb 16, 2022

gopherbot added the FrozenDueToAge label Feb 16, 2022

rsc unassigned randall77 Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/compile: inconsistent behaviour in multiple assignment #23017

cmd/compile: inconsistent behaviour in multiple assignment #23017

zigo101 commented Dec 6, 2017

ianlancetaylor commented Dec 6, 2017

randall77 commented Dec 6, 2017

griesemer commented Dec 6, 2017 •

edited

Loading

griesemer commented Dec 6, 2017

mdempsky commented Dec 6, 2017 •

edited

Loading

mdempsky commented Dec 6, 2017

mdempsky commented Dec 6, 2017

mdempsky commented Dec 6, 2017

randall77 commented Dec 11, 2017 •

edited

Loading

randall77 commented Dec 12, 2017

randall77 commented May 6, 2019

cuonglm commented Oct 4, 2019

cuonglm commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 11, 2019

gopherbot commented Oct 12, 2020

gopherbot commented Jan 3, 2021

cmd/compile: inconsistent behaviour in multiple assignment #23017

cmd/compile: inconsistent behaviour in multiple assignment #23017

Comments

zigo101 commented Dec 6, 2017

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

Does this issue reproduce with the latest release?

What did you do?

What did you expect to see?

What did you see instead?

ianlancetaylor commented Dec 6, 2017

randall77 commented Dec 6, 2017

griesemer commented Dec 6, 2017 • edited Loading

griesemer commented Dec 6, 2017

mdempsky commented Dec 6, 2017 • edited Loading

mdempsky commented Dec 6, 2017

mdempsky commented Dec 6, 2017

mdempsky commented Dec 6, 2017

randall77 commented Dec 11, 2017 • edited Loading

randall77 commented Dec 12, 2017

randall77 commented May 6, 2019

cuonglm commented Oct 4, 2019

cuonglm commented Oct 4, 2019

gopherbot commented Oct 4, 2019

gopherbot commented Oct 11, 2019

gopherbot commented Oct 12, 2020

gopherbot commented Jan 3, 2021

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

griesemer commented Dec 6, 2017 •

edited

Loading

mdempsky commented Dec 6, 2017 •

edited

Loading

randall77 commented Dec 11, 2017 •

edited

Loading