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.

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

cmd/compile: inconsistent behaviour in multiple assignment #23017

Open
go101 opened this issue Dec 6, 2017 · 17 comments
Open

cmd/compile: inconsistent behaviour in multiple assignment #23017

go101 opened this issue Dec 6, 2017 · 17 comments
Assignees
Labels
Milestone

Comments

@go101
Copy link

@go101 go101 commented Dec 6, 2017

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.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 6, 2017

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

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Dec 6, 2017
@randall77
Copy link
Contributor

@randall77 randall77 commented Dec 6, 2017

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

@griesemer griesemer commented Dec 6, 2017

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

@griesemer griesemer commented Dec 6, 2017

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

@mdempsky mdempsky commented Dec 6, 2017

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.

@griesemer griesemer added NeedsFix and removed Documentation labels Dec 6, 2017
@griesemer griesemer modified the milestones: Unplanned, Go1.11 Dec 6, 2017
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Dec 6, 2017

Related discussion: #15620.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Dec 6, 2017

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

@mdempsky mdempsky commented Dec 6, 2017

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

@randall77
Copy link
Contributor

@randall77 randall77 commented Dec 11, 2017

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

@randall77 randall77 commented Dec 12, 2017

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

@randall77
Copy link
Contributor

@randall77 randall77 commented May 6, 2019

Punting again, too late for 1.13.

@randall77 randall77 modified the milestones: Go1.13, Go1.14 May 6, 2019
@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Oct 4, 2019

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

@cuonglm cuonglm commented Oct 4, 2019

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

@gopherbot gopherbot commented Oct 4, 2019

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

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 11, 2019

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

gopherbot pushed a commit that referenced this issue Oct 12, 2019
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
Copy link

@gopherbot gopherbot commented Oct 12, 2020

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

gopherbot pushed a commit that referenced this issue Oct 13, 2020
"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
Copy link

@gopherbot gopherbot commented Jan 3, 2021

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

gopherbot pushed a commit that referenced this issue Jan 3, 2021
After the previous cleanup/optimization CLs, ascompatee now correctly
handles map assignments too. So remove the code from order.mapAssign,
which causes us to assign to the map at the wrong point during
execution. It's not every day you get to fix an issue by only removing
code.

Thanks to Cuong Manh Le for test cases and continually following up on
this issue.

Passes toolstash -cmp. (Apparently the standard library never uses
tricky map assignments. Go figure.)

Fixes #23017.

Change-Id: Ie0728103d59d884d00c1c050251290a2a46150f9
Reviewed-on: https://go-review.googlesource.com/c/go/+/281172
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants