Reviewers: ken2,
Message:
Hello ken2 (cc: golang-dev@googlegroups.com),
I'd like you to review this change to
https://code.google.com/p/go/
Description:
cmd/gc: fix eval order in select
Ordinary variable load was assumed to be not worth saving,
but not if one of the function calls later might change
its value.
Fixes issue 4313.
Please review this at https://codereview.appspot.com/6997047/
Affected files:
M src/cmd/gc/order.c
M src/cmd/gc/select.c
M src/cmd/gc/subr.c
A test/fixedbugs/issue4313.go
Index: src/cmd/gc/order.c
===================================================================
--- a/src/cmd/gc/order.c
+++ b/src/cmd/gc/order.c
@@ -276,11 +276,11 @@
case OSELRECV2:
orderexprinplace(&r->left);
orderexprinplace(&r->ntest);
- orderexpr(&r->right->left, out);
+ orderexpr(&r->right->left, &l->n->ninit);
break;
case OSEND:
- orderexpr(&r->left, out);
- orderexpr(&r->right, out);
+ orderexpr(&r->left, &l->n->ninit);
+ orderexpr(&r->right, &l->n->ninit);
break;
}
}
Index: src/cmd/gc/select.c
===================================================================
--- a/src/cmd/gc/select.c
+++ b/src/cmd/gc/select.c
@@ -297,15 +297,15 @@
setlineno(cas);
n = cas->left;
r = nod(OIF, N, N);
- r->nbody = cas->ninit;
+ r->ninit = cas->ninit;
cas->ninit = nil;
if(n != nil) {
- r->nbody = concat(r->nbody, n->ninit);
+ r->ninit = concat(r->ninit, n->ninit);
n->ninit = nil;
}
if(n == nil) {
// selectdefault(sel *byte);
- r->ntest = mkcall("selectdefault", types[TBOOL], &init, var);
+ r->ntest = mkcall("selectdefault", types[TBOOL], &r->ninit, var);
} else {
switch(n->op) {
default:
@@ -313,25 +313,25 @@
case OSEND:
// selectsend(sel *byte, hchan *chan any, elem *any) (selected bool);
- n->left = safeexpr(n->left, &r->ninit);
+ n->left = localexpr(safeexpr(n->left, &r->ninit), n->left->type,
&r->ninit);
n->right = localexpr(n->right, n->left->type->type, &r->ninit);
n->right = nod(OADDR, n->right, N);
n->right->etype = 1; // pointer does not escape
typecheck(&n->right, Erv);
r->ntest = mkcall1(chanfn("selectsend", 2, n->left->type),
types[TBOOL],
- &init, var, n->left, n->right);
+ &r->ninit, var, n->left, n->right);
break;
case OSELRECV:
// selectrecv(sel *byte, hchan *chan any, elem *any) (selected bool);
r->ntest = mkcall1(chanfn("selectrecv", 2, n->right->left->type),
types[TBOOL],
- &init, var, n->right->left, n->left);
+ &r->ninit, var, n->right->left, n->left);
break;
case OSELRECV2:
// selectrecv2(sel *byte, hchan *chan any, elem *any, received *bool)
(selected bool);
r->ntest = mkcall1(chanfn("selectrecv2", 2, n->right->left->type),
types[TBOOL],
- &init, var, n->right->left, n->left, n->ntest);
+ &r->ninit, var, n->right->left, n->left, n->ntest);
break;
}
}
Index: src/cmd/gc/subr.c
===================================================================
--- a/src/cmd/gc/subr.c
+++ b/src/cmd/gc/subr.c
@@ -2040,11 +2040,13 @@
/*
* return n in a local variable of type t if it is not already.
+ * the value is guaranteed not to change except by direct
+ * assignment to it.
*/
Node*
localexpr(Node *n, Type *t, NodeList **init)
{
- if(n->op == ONAME &&
+ if(n->op == ONAME && !n->addrtaken &&
(n->class == PAUTO || n->class == PPARAM || n->class == PPARAMOUT) &&
convertop(n->type, t, nil) == OCONVNOP)
return n;
Index: test/fixedbugs/issue4313.go
===================================================================
new file mode 100644
--- /dev/null
+++ b/test/fixedbugs/issue4313.go
@@ -0,0 +1,28 @@
+// run
+
+// Copyright 2012 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Order of operations in select.
+
+package main
+
+func main() {
+ c := make(chan int, 1)
+ x := 0
+ select {
+ case c <- x: // should see x = 0, not x = 42 (after makec)
+ case <-makec(&x): // should be evaluated only after c and x on previous
line
+ }
+ y := <-c
+ if y != 0 {
+ panic(y)
+ }
+}
+
+func makec(px *int) chan bool {
+ if false { for {} }
+ *px = 42
+ return make(chan bool, 0)
+}
