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

spec: order of evaluation vs panicking #23735

Open
mdempsky opened this issue Feb 7, 2018 · 5 comments
Open

spec: order of evaluation vs panicking #23735

mdempsky opened this issue Feb 7, 2018 · 5 comments
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Member

mdempsky commented Feb 7, 2018

It's possible to write expressions that, depending on order of evaluation, may panic for different reasons. I think this is fine, but it might be worth mentioning or providing examples to make clearer / less surprising to users.

For example, consider:

var p *[0]int
var i int

(*p)[i] = 0

Under cmd/compile, this snippet produces an "invalid memory address or nil pointer dereference" panic; whereas under gccgo, it produces an "index out of range" panic.

--

Another example is:

var p *int
var x int

_ = *p / x

Both cmd/compile and gccgo produce a nil pointer dereference panic, but the Go spec appears to also permit division by zero.

--

The latter example is relevant to #23661, because of:

var m = map[interface{}]int{0: 0}  // non-empty map to workaround #23734
var k []int
var x int

m[k] /= x

Today, the last statement is compiled into roughly:

tmp := *mapaccess1(m, k)
if x == 0 {
    panicdivide()
}
*mapassign(m, k) = tmp / x

It would be nice to optimize that to

if x == 0 {
    panicdivide()
}
*mapassign(m, k) /= x

But that would change the panic from "hash of unhashable type []int" to "division by zero". This change appears valid under the Go spec, but it seems worth clarifying whether that's the case.

@mdempsky
Copy link
Member Author

mdempsky commented Feb 7, 2018

/cc @griesemer @ianlancetaylor

@randall77
Copy link
Contributor

The Run-time panics section of the spec seems to imply to me that the nature of the panic is irrelevant, only whether there is one or not.
i.e. for the code:

var p *int
_ = *p

My reading of the spec would allow a panic with a "division by zero" error.
Not that we would want to do that, but no one should be programmatically depending on the error text.

@griesemer
Copy link
Contributor

I'm ok with adding clarifying examples to the spec.

@griesemer griesemer self-assigned this Feb 12, 2018
@griesemer griesemer added this to the Go1.11 milestone Feb 12, 2018
@gopherbot
Copy link

Change https://golang.org/cl/91557 mentions this issue: cmd/compile: avoid extra mapaccess in "m[k] op= r"

gopherbot pushed a commit that referenced this issue Mar 12, 2018
Currently, order desugars map assignment operations like

    m[k] op= r

into

    m[k] = m[k] op r

which in turn is transformed during walk into:

    tmp := *mapaccess(m, k)
    tmp = tmp op r
    *mapassign(m, k) = tmp

However, this is suboptimal, as we could instead produce just:

    *mapassign(m, k) op= r

One complication though is if "r == 0", then "m[k] /= r" and "m[k] %=
r" will panic, and they need to do so *before* calling mapassign,
otherwise we may insert a new zero-value element into the map.

It would be spec compliant to just emit the "r != 0" check before
calling mapassign (see #23735), but currently these checks aren't
generated until SSA construction. For now, it's simpler to continue
desugaring /= and %= into two map indexing operations.

Fixes #23661.

Change-Id: I46e3739d9adef10e92b46fdd78b88d5aabe68952
Reviewed-on: https://go-review.googlesource.com/91557
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 29, 2018
@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Unplanned Jun 29, 2018
@gopherbot
Copy link

Change https://go.dev/cl/418097 mentions this issue: [dev.unified] test: relax panic message expectations

gopherbot pushed a commit that referenced this issue Jul 19, 2022
In this test, traditionally the comparison `*l == r[0]` was left as a
comparison between `*l` (type `any`) and `r[0]` (type `*int`), and the
rest of the compiler needed to handle mixed-typed comparisons.
However, this means more complexity for wiring up explicit rtypes.

To simplify rtype handling, the next CL will change unified IR to
instead handle the expression as `*l == any(r[0])`. However, a
consequence of this currently is that walk will now sequence the
`any(r[0])` expression first, because it involves a
concrete-to-interface conversion. And in turn, this means the `r[0]`
panic ("index out of bounds") will take priority over the `*l`
panic ("nil pointer dereference").

This is a change in user-visible semantics in some cases, but the Go
spec leaves this unspecified, so it shouldn't be an issue. Note also:
gccgo has the same behavior (i.e., panicking on index out of bounds,
not nil pointer dereference), and cmd/compile also already has the
same behavior when the interface conversion is explicit (as in the
added "nil pointer dereference #3" test case).

Updates #23735.
Updates #32187.

Change-Id: I49e5dcca85b4680f9c8780ef0013e64254d38fe5
Reviewed-on: https://go-review.googlesource.com/c/go/+/418097
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
In this test, traditionally the comparison `*l == r[0]` was left as a
comparison between `*l` (type `any`) and `r[0]` (type `*int`), and the
rest of the compiler needed to handle mixed-typed comparisons.
However, this means more complexity for wiring up explicit rtypes.

To simplify rtype handling, the next CL will change unified IR to
instead handle the expression as `*l == any(r[0])`. However, a
consequence of this currently is that walk will now sequence the
`any(r[0])` expression first, because it involves a
concrete-to-interface conversion. And in turn, this means the `r[0]`
panic ("index out of bounds") will take priority over the `*l`
panic ("nil pointer dereference").

This is a change in user-visible semantics in some cases, but the Go
spec leaves this unspecified, so it shouldn't be an issue. Note also:
gccgo has the same behavior (i.e., panicking on index out of bounds,
not nil pointer dereference), and cmd/compile also already has the
same behavior when the interface conversion is explicit (as in the
added "nil pointer dereference golang#3" test case).

Updates golang#23735.
Updates golang#32187.

Change-Id: I49e5dcca85b4680f9c8780ef0013e64254d38fe5
Reviewed-on: https://go-review.googlesource.com/c/go/+/418097
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants