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/gc: internal error: foo recorded as live on entry #7561

Closed
dvyukov opened this issue Mar 17, 2014 · 10 comments
Closed

cmd/gc: internal error: foo recorded as live on entry #7561

dvyukov opened this issue Mar 17, 2014 · 10 comments

Comments

@dvyukov
Copy link
Member

dvyukov commented Mar 17, 2014

go version devel +32b8c26e4414 Mon Mar 17 09:35:04 2014 +1100 linux/amd64

$ go get github.com/karalabe/iris
commit 72957eb03e5205f73fe287e6f53a5690bdd00c23

$ cd gopath/src/github.com/karalabe/iris/proto/iris
$ go test -race

# github.com/karalabe/iris/proto/iris
./broadcast_test.go:72: internal error: testBroadcast autotmp_0473 (type *[]*Connection)
recorded as live on entry
./pubsub_test.go:64: internal error: testPubSub autotmp_0559 (type *[]*Connection)
recorded as live on entry
./reqrep_test.go:77: internal error: testReqRep autotmp_0646 (type *[]*Connection)
recorded as live on entry
./tunnel_test.go:86: internal error: testTunnel autotmp_0721 (type *[]*Connection)
recorded as live on entry
FAIL    github.com/karalabe/iris/proto/iris [build failed]
@dvyukov
Copy link
Member Author

dvyukov commented Mar 17, 2014

Comment 1:

With Go1.2.1 this test crashes with nil deref instead. And the nil deref looks very like
a codegen issue.
So when the build issue is fixed we need to ensure that the test does not crash due to
codegen issues.

@dvyukov
Copy link
Member Author

dvyukov commented Mar 17, 2014

Comment 2:

The following change fixes *both* compiler error on tip and runtime crash in 1.2
diff --git a/proto/iris/broadcast_test.go b/proto/iris/broadcast_test.go
index d6685a9..2d2b522 100644
--- a/proto/iris/broadcast_test.go
+++ b/proto/iris/broadcast_test.go
@@ -104,12 +104,12 @@ func testBroadcast(t *testing.T, nodes, conns, msgs int) {
        liveHands[i] = make([]*broadcaster, conns)
        liveConns[i] = make([]*Connection, conns)
        for j := 0; j < conns; j++ {
-           var err error
            liveHands[i][j] = &broadcaster{make(chan []byte, nodes*conns*msgs)}
-           liveConns[i][j], err = node.Connect(cluster, liveHands[i][j])
+           conn, err := node.Connect(cluster, liveHands[i][j])
            if err != nil {
                t.Fatalf("failed to connect to the iris overlay: %v.", err)
            }
+           liveConns[i][j] = conn
            defer func(conn *Connection) {
                if err := conn.Close(); err != nil {
                    t.Fatalf("failed to close iris connection: %v.", err)
diff --git a/proto/iris/pubsub_test.go b/proto/iris/pubsub_test.go
index 5e29610..d1c25eb 100644
--- a/proto/iris/pubsub_test.go
+++ b/proto/iris/pubsub_test.go
@@ -97,12 +97,12 @@ func testPubSub(t *testing.T, nodes, conns, msgs int) {
        liveHands[i] = make([]*subscriber, conns)
        liveConns[i] = make([]*Connection, conns)
        for j := 0; j < conns; j++ {
-           var err error
            // Connect to the iris network
-           liveConns[i][j], err = node.Connect(cluster, nil)
+           conn, err := node.Connect(cluster, nil)
            if err != nil {
                t.Fatalf("failed to connect to the iris overlay: %v.", err)
            }
+           liveConns[i][j] = conn
            defer func(conn *Connection) {
                if err := conn.Close(); err != nil {
                    t.Fatalf("failed to close iris connection: %v.", err)
diff --git a/proto/iris/reqrep_test.go b/proto/iris/reqrep_test.go
index d589c41..d04c833 100644
--- a/proto/iris/reqrep_test.go
+++ b/proto/iris/reqrep_test.go
@@ -109,12 +109,12 @@ func testReqRep(t *testing.T, nodes, conns, reqs int) {
        liveHands[i] = make([]*requester, conns)
        liveConns[i] = make([]*Connection, conns)
        for j := 0; j < conns; j++ {
-           var err error
            liveHands[i][j] = &requester{i, 0}
-           liveConns[i][j], err = node.Connect(cluster, liveHands[i][j])
+           conn, err := node.Connect(cluster, liveHands[i][j])
            if err != nil {
                t.Fatalf("failed to connect to the iris overlay: %v.", err)
            }
+           liveConns[i][j] = conn
            defer func(conn *Connection) {
                if err := conn.Close(); err != nil {
                    t.Fatalf("failed to close iris connection: %v.", err)
diff --git a/proto/iris/tunnel_test.go b/proto/iris/tunnel_test.go
index 053548d..8e27d6e 100644
--- a/proto/iris/tunnel_test.go
+++ b/proto/iris/tunnel_test.go
@@ -118,12 +118,12 @@ func testTunnel(t *testing.T, nodes, conns, tuns, msgs int) {
        liveHands[i] = make([]*tunneler, conns)
        liveConns[i] = make([]*Connection, conns)
        for j := 0; j < conns; j++ {
-           var err error
            liveHands[i][j] = &tunneler{i, 0}
-           liveConns[i][j], err = node.Connect(cluster, liveHands[i][j])
+           conn, err := node.Connect(cluster, liveHands[i][j])
            if err != nil {
                t.Fatalf("failed to connect to the iris overlay: %v.", err)
            }
+           liveConns[i][j] = conn
            defer func(conn *Connection) {
                if err := conn.Close(); err != nil {
                    t.Fatalf("failed to close iris connection: %v.", err)

@remyoudompheng
Copy link
Contributor

Comment 3:

I have investigated this: here is a minimal example:
package p
var connect func() (int, error)
var conns = make(map[int][]int)
func TestMultipleReturnToMap() {
      var i, j int
      var err error
      conns[i][j], err = connect()
      _ = err
}
The issues arises because multiple returns "a, b = f()" is walked to:
{
    CALL f
    a = SP[n0]
    b = SP[n1]
}
Then because racewalk cannot insert instrumentation between CALL and y = SP[n], it
inserts them at the end. However, map indexing is not an actual assignement so if you
have "m[x], y = f()" it becomes:
{
    CALL f
    tmp = SP[n0]
    y = SP[n1]
    m[x] = tmp
}
and racewalk again puts all instrumentation at the end. Thus is becomes:
{
    CALL f
    tmp = SP[n0]
    y = SP[n1]
    *p = tmp 
    // init of previous statement
    p = mapassign1(m, x)
    raceread(...); readwrite(...)   
}
Instead it should be:
{
    CALL f
    tmp = SP[n0]
    y = SP[n1]
    <instrumentation of y if it is on the heap>
    // instrumentation of m[x] = tmp
    p = mapassign1(m, x)
    raceread(m); readwrite(p)
    *p = tmp 
}
I'm not sure yet how to do this properly, I need a closer look later in the week.

@remyoudompheng
Copy link
Contributor

Comment 4:

Wall, you may want to try https://golang.org/cl/76520045 but it feels a bit
hackish.

Status changed to Started.

@dvyukov
Copy link
Member Author

dvyukov commented Mar 18, 2014

Comment 5:

> However, map indexing is not an actual assignement so if you have "m[x], y = f()" it
becomes:
> {
>     CALL f
>     tmp = SP[n0]
>     y = SP[n1]
>     m[x] = tmp
> }
Is "m[x] = tmp" a separate statement here? Or a part of a more complex statement?
Gc should have been lowered it to a separate statement. Then things would be much
simpler. But we probably don't wan't to do such changes before release...

Owner changed to @remyoudompheng.

@dvyukov
Copy link
Member Author

dvyukov commented Mar 18, 2014

Comment 6:

https://golang.org/cl/76520045 makes sense to me.
We also need a test.

@rsc
Copy link
Contributor

rsc commented Apr 3, 2014

Comment 7:

Seems to be working now, because order.c now moves mapassigns to separate statements.
g% cat >x.go
package p
var connect func() (int, error)
var conns = make(map[int][]int)
func TestMultipleReturnToMap() {
      var i, j int
      var err error
      conns[i][j], err = connect()
      _ = err
}
g% go tool 6g -race x.go
g%
I still believe 76520045 should be rewritten to do what I suggested, but it doesn't seem
to be release-critical.

Labels changed: removed release-go1.3.

@gopherbot
Copy link

Comment 8:

CL https://golang.org/cl/76520045 references this issue.

@rsc
Copy link
Contributor

rsc commented Apr 17, 2014

Comment 9:

This is working for me at tip.

Status changed to Fixed.

@gopherbot
Copy link

CL https://golang.org/cl/11713 mentions this issue.

rsc added a commit that referenced this issue Jun 30, 2015
Fixes #7561 correctly.
Fixes #9137.

Change-Id: I7f27e199d7101b785a7645f789e8fe41a405a86f
Reviewed-on: https://go-review.googlesource.com/11713
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
@golang golang locked and limited conversation to collaborators Jun 28, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants