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: wrong evaluation order for map assignment [Go1.8] #22881

Closed
randall77 opened this Issue Nov 26, 2017 · 12 comments

Comments

Projects
None yet
8 participants
@randall77
Copy link
Contributor

randall77 commented Nov 26, 2017

package main

func main() {
	m := map[int]int{}
	defer func() {
		recover()
		println(len(m))
	}()
	m[5] = m[5] + *p
}

var p *int

This should print 0, not 1. The nil ptr panic should happen before the insert into m.
It prints 0 up to go 1.7, but starting at go 1.8 it prints 1.
Same thing happens if I use s[0] on a nil slice instead of *p on a nil pointer.

The bug goes away if I do tmp := m[5] + *p; m[5] = tmp.

I'm going to label this as a backportable 1.9.3 issue so we can discuss (and maybe also 1.8.6, but github won't let me multi-milestone an issue), but I kind of lean toward just fixing this in 1.10. It's pretty esoteric, survived more than a release cycle without a bug report, and it is easy to work around.

It's also possible I'm not reading the spec correctly, and this is allowed behavior. @griesemer

Doesn't seem to be SSA related - the AST already has the ops out of order on input to SSA.

@randall77 randall77 added this to the Go1.9.3 milestone Nov 26, 2017

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Nov 26, 2017

Slightly simpler version, as a playground link: https://play.golang.org/p/5hasX7yOX1

@griesemer

This comment has been minimized.

Copy link
Contributor

griesemer commented Nov 26, 2017

Looks like a bug to me. The spec says that the right-hand side is first fully evaluated before any assignments take place. A nil-pointer deref causes a runtime panic similar to calling panic() which (immediately) terminates the surrounding function.

@mdempsky

This comment has been minimized.

Copy link
Member

mdempsky commented Nov 29, 2017

Yeah, this is probably an order.go issue. We can't call mapassign until we know the RHS will evaluate without panicking.

At least OIND, OINDEX, ODIV, and OMOD can panic, but are currently allowed after the mapassign call.

@randall77 randall77 self-assigned this Dec 4, 2017

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Dec 4, 2017

Change https://golang.org/cl/81817 mentions this issue: cmd/compile: fix map assignment with panicking right-hand side

@gopherbot gopherbot closed this in dd7cbf3 Dec 5, 2017

@randall77

This comment has been minimized.

Copy link
Contributor Author

randall77 commented Dec 5, 2017

Reopening for backport consideration (both 1.9.3 and 1.8.6).

@randall77 randall77 reopened this Dec 5, 2017

@randall77 randall77 modified the milestones: Go1.9.3, Go1.8.6 Dec 5, 2017

@randall77 randall77 changed the title cmd/compile: wrong evaluation order for map assignment cmd/compile: wrong evaluation order for map assignment [Go1.8] Dec 5, 2017

@go101

This comment has been minimized.

Copy link

go101 commented Dec 6, 2017

This is not only for map

package main

var x int
func f2() *int {
  println("bye") // this line shouldn't be output.
  return &x
}

func main() {
        var p *int
        *f2() = *p
}

[update] Aha, this may be some different.

@go101

This comment has been minimized.

Copy link

go101 commented Dec 6, 2017

however, if we view left m[key]as *GetMapValueAddr(key), then the example in the first comment may be also viewed as not a bug.

@go101

This comment has been minimized.

Copy link

go101 commented Dec 6, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Dec 6, 2017

@go101 The order of evaluation rules are at https://golang.org/ref/spec#Order_of_evaluation. There is no specified order between the function call on the left hand side and the indirection on the right hand side. It's clear that the indirection must happen before the assignment, but there is no requirement that the indirection happen before the function call.

@go101

This comment has been minimized.

Copy link

go101 commented Dec 6, 2017

Then this

package main

import "fmt"

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

The spec says m[2]=1 happens before *p=2, so the print result should be 1 instead.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Dec 6, 2017

Let's take detailed discussion of order of evaluation away from this issue, which is about a specific bug. Thanks.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

ianlancetaylor commented Jan 11, 2018

Closing proposed backport per discussion in #23005. (This remains fixed in the upcoming 1.10 release.)

@andybons andybons removed this from the Go1.8.6 milestone Jan 23, 2018

@golang golang locked and limited conversation to collaborators Jan 23, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.