{"action":"created","issue":{"url":"https://api.github.com/repos/golang/go/issues/27772","repository_url":"https://api.github.com/repos/golang/go","labels_url":"https://api.github.com/repos/golang/go/issues/27772/labels{/name}","comments_url":"https://api.github.com/repos/golang/go/issues/27772/comments","events_url":"https://api.github.com/repos/golang/go/issues/27772/events","html_url":"https://github.com/golang/go/issues/27772","id":362112811,"node_id":"MDU6SXNzdWUzNjIxMTI4MTE=","number":27772,"title":"cmd/compile/internal/gc: generalize self-assignment handling in escape analysis","user":{"login":"Quasilyte","id":6286655,"node_id":"MDQ6VXNlcjYyODY2NTU=","avatar_url":"https://avatars3.githubusercontent.com/u/6286655?v=4","gravatar_id":"","url":"https://api.github.com/users/Quasilyte","html_url":"https://github.com/Quasilyte","followers_url":"https://api.github.com/users/Quasilyte/followers","following_url":"https://api.github.com/users/Quasilyte/following{/other_user}","gists_url":"https://api.github.com/users/Quasilyte/gists{/gist_id}","starred_url":"https://api.github.com/users/Quasilyte/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Quasilyte/subscriptions","organizations_url":"https://api.github.com/users/Quasilyte/orgs","repos_url":"https://api.github.com/users/Quasilyte/repos","events_url":"https://api.github.com/users/Quasilyte/events{/privacy}","received_events_url":"https://api.github.com/users/Quasilyte/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-09-20T10:04:23Z","updated_at":"2018-09-20T10:05:07Z","closed_at":null,"author_association":"CONTRIBUTOR","body":"Right now, there are at least 2 quite adhoc patterns that are recognized by\r\nescape analysis as safe. It skips detailed analysis when it sees\r\nself-assignment statement that does not introduce new pointers that need tracking.\r\n\r\nInitially, only some simple self-slicing assignments like this were handled:\r\n\r\n```go\r\nx.buf = x.buf[a:b]\r\n```\r\n\r\nLater, some more patterns were added, so these are recognized, too:\r\n\r\n```go\r\nx.ptr1 = x.ptr2\r\nval.pointers[i] = val.pointers[j]\r\n```\r\n\r\nThe problem with them is that they are very fragile and can't match expressions\r\nthat are different from the simplest cases, but still represents self-assignments:\r\n\r\n```go\r\nx.a.b.buf = x.a.b.buf[a:b]\r\n```\r\n\r\nIt is possible to generalize all self-assignment patterns with a concept of \"base object\".\r\nWhat we see above is a matching of object field assignment to the object itself.\r\nThe base object is that object, the one that contains referenced field.\r\n\r\nFor the simplest cases, base object is just 1 syntactical level \"above\":\r\n\r\n```go\r\nbase(x.y)     // => x\r\nbase(x.y.z)   // => x.y\r\nbase(x[i])    // => x\r\nbase(x[i][j]) // => x[i]\r\n```\r\n\r\nFor cases where expression effectively returns the same object, we can skip several levels:\r\n\r\n```go\r\nbase(x[:])    => x\r\nbase(x[:][:]) => x\r\n```\r\n\r\nGiven `base` function, we can express self-assignment as (pseudo Go):\r\n\r\n```go\r\n// See samesafeexpr from cmd/compile/internal/gc/typecheck.go\r\nreturn samesafeexpr(base(dst), base(src))\r\n```\r\n\r\nThis covers all patterns above, plus a few more.\r\nAs a bonus, it also solves trivial `*p = *p` case (see https://github.com/golang/go/issues/5919):\r\n\r\n```go\r\nfunc j(p *string) {\r\n\t*p = *p\r\n}\r\n```\r\n\r\nMore interesting examples of self-assignments that were not recognized until generalization:\r\n\r\n```go\r\ntype node struct { next *node }\r\nfunc createLoop(n *node) {\r\n\tn.next = n // n was leaking param before\r\n}\r\n```\r\n\r\n```go\r\ntype foo struct {\r\n\tpi *int\r\n\ti  int\r\n}\r\nfunc (f *foo) example() {\r\n\tf.pi = &f.i // f was leaking param before\r\n}\r\n```\r\n\r\nThis solution (if it is correct or can be made correct):\r\n- Makes self-assignment check less adhoc, more powerfull (hopefully, more useful)\r\n- It makes implementation simpler, reduces code duplication\r\n\r\n<hr>\r\n\r\nCollect new escape analysis results info:\r\n\r\n```bash\r\ngo build -a -gcflags=-m ./src/... 2>&1 | grep 'does not escape' > new_noescapes.txt\r\ngo build -a -gcflags=-m ./src/... 2>&1 | grep 'leaking param' > new_leakparam.txt\r\n```\r\n\r\nAnd now with unpatched `go tool compile`:\r\n\r\n```bash\r\ngo build -a -gcflags=-m ./src/... 2>&1 | grep 'does not escape' > old_noescapes.txt\r\ngo build -a -gcflags=-m ./src/... 2>&1 | grep 'leaking param' > old_leakparam.txt\r\n```\r\n\r\nNow it is possible to do some comparisons.\r\n\r\nNew non-escaping values/parameters:\r\n\r\n```\r\nsrc/net/net.go:687:7: (*Buffers).consume v does not escape\r\nsrc/net/net.go:674:7: (*Buffers).Read v does not escape\r\nsrc/internal/poll/fd.go:48:14: consume v does not escape\r\nsrc/compress/flate/inflate.go:325:10: (*decompressor).nextBlock &f.h1 does not escape\r\nsrc/compress/flate/inflate.go:326:10: (*decompressor).nextBlock &f.h2 does not escape\r\nsrc/cmd/compile/internal/gc/const.go:668:15: evconst &nl does not escape\r\nsrc/container/ring/ring.go:121:7: (*Ring).Len r does not escape\r\nsrc/cmd/compile/internal/types/type.go:727:13: (*Type).copy &nt does not escape\r\n```\r\n\r\nSince `ring.Ring.Len` receiver no longer escapes, we can try to verify and measure it with benchmarks:\r\n\r\n```go\r\npackage foo\r\n\r\nimport (\r\n\t\"container/ring\"\r\n\t\"testing\"\r\n)\r\n\r\nfunc BenchmarkRingLen(b *testing.B) {\r\n\tfor i := 0; i < b.N; i++ {\r\n\t\tvar r ring.Ring\r\n\t\t_ = r.Len()\r\n\t}\r\n}\r\n```\r\n\r\nOld is unpatched escape analysis with leaking `r`. New is non-leaking `r`:\r\n\r\n```\r\nname       old time/op    new time/op    delta\r\nRingLen-8    35.6ns ± 6%     3.0ns ± 0%   -91.46%  (p=0.000 n=10+10)\r\n\r\nname       old alloc/op   new alloc/op   delta\r\nRingLen-8     32.0B ± 0%      0.0B       -100.00%  (p=0.000 n=10+10)\r\n\r\nname       old allocs/op  new allocs/op  delta\r\nRingLen-8      1.00 ± 0%      0.00       -100.00%  (p=0.000 n=10+10)\r\n```\r\n\r\nHere is `ring.Ring.Len` method implementation, for the reference:\r\n\r\nhttps://golang.org/src/container/ring/ring.go?s=2869:2893#L111\r\n\r\n<hr>\r\n\r\nFor additional examples, see proposed test suite extension:\r\n\r\n```go\r\npackage foo\r\n\r\nvar sink interface{}\r\n\r\nfunc length(xs []byte) int { // ERROR \"leaking param: xs\"\r\n\tsink = xs // ERROR \"xs escapes to heap\"\r\n\treturn len(xs)\r\n}\r\n\r\nfunc zero() int { return 0 }\r\n\r\ntype buffer struct {\r\n\tarr        [64]byte\r\n\tbuf1       []byte\r\n\tbuf2       []byte\r\n\tbufPtr1    *[]byte\r\n\tbufPtr2    *[]byte\r\n\tbufList    [][]byte\r\n\tbufArr     [5][]byte\r\n\tbufPtrList []*[]byte\r\n\tstr1       string\r\n\tstr2       string\r\n\tnext       *buffer\r\n\tbuffers    []*buffer\r\n}\r\n\r\nfunc (b *buffer) getNext() *buffer { // ERROR \"leaking param: b to result ~r0 level=1\"\r\n\treturn b.next\r\n}\r\n\r\n// When referring to the \"old\" implementation, cases that are covered in\r\n// escape2.go are implied.\r\n// Most tests here are based on those tests, but with slight changes,\r\n// like extra selector expression level.\r\n\r\nfunc (b *buffer) noEsc() { // ERROR \"\\(\\*buffer\\).noEsc b does not escape\"\r\n\t// Like original slicing self-assignment test, but with additional\r\n\t// slicing expressions inside the RHS.\r\n\tb.buf1 = b.buf1[:][1:2]        // ERROR \"ignoring self-assignment to b.buf1$\"\r\n\tb.buf1 = b.buf1[1:][1:2:3]     // ERROR \"ignoring self-assignment to b.buf1$\"\r\n\tb.buf1 = b.buf2[:2][1:][1:2]   // ERROR \"ignoring self-assignment to b.buf1$\"\r\n\tb.buf1 = b.buf2[:][1:2][1:2:3] // ERROR \"ignoring self-assignment to b.buf1$\"\r\n\r\n\t// The \"left\" (base) part is generalized and can be arbitrary,\r\n\t// as long as it doesn't affect memory.\r\n\t// Basically, these cases add \"next\" in the buf accessing chain.\r\n\tb.next.buf1 = b.next.buf1[1:2]             // ERROR \"ignoring self-assignment to b.next.buf1$\"\r\n\tb.next.buf1 = b.next.buf1[1:2:3]           // ERROR \"ignoring self-assignment to b.next.buf1$\"\r\n\tb.next.buf1 = b.next.buf2[1:2]             // ERROR \"ignoring self-assignment to b.next.buf1$\"\r\n\tb.next.next.buf1 = b.next.next.buf2[1:2:3] // ERROR \"ignoring self-assignment to b.next.next.buf1$\"\r\n\r\n\t// Indexing functionally is almost identical to field accessing.\r\n\t// It's permitted to have different trailing indexes just as it's\r\n\t// permitted to have different trailing selectors.\r\n\tindex1 := 10\r\n\tindex2 := index1\r\n\tb.bufList[0] = b.bufList[1][1:2]             // ERROR \"ignoring self-assignment to b.bufList\\[0\\]$\"\r\n\tb.bufList[index1] = b.bufList[index2][1:2:3] // ERROR \"ignoring self-assignment to b.bufList\\[index1]$\"\r\n\tb.bufArr[0] = b.bufArr[1+1][1:2]             // ERROR \"ignoring self-assignment to b.bufArr\\[0]$\"\r\n\t*b.bufPtrList[2] = (*b.bufPtrList[1])[1:2:3] // ERROR \"ignoring self-assignment to \\*b.bufPtrList\\[2\\]$\"\r\n\t// Same indexes should work as well.\r\n\tb.bufList[0] = b.bufList[0][1:2]             // ERROR \"ignoring self-assignment to b.bufList\\[0\\]$\"\r\n\tb.bufList[index1] = b.bufList[index1][1:2:3] // ERROR \"ignoring self-assignment to b.bufList\\[index1]$\"\r\n\tb.bufArr[1+1] = b.bufArr[1+1][1:2]           // ERROR \"ignoring self-assignment to b.bufArr\\[1 \\+ 1\\]$\"\r\n\t*b.bufPtrList[2] = (*b.bufPtrList[2])[1:2:3] // ERROR \"ignoring self-assignment to \\*b.bufPtrList\\[2\\]$\"\r\n\t// Works for chained indexing as well, but indexing in base objects must match.\r\n\tb.buffers[0].bufList[0] = b.buffers[0].bufList[0][1:2]                           // ERROR \"ignoring self-assignment to b.buffers\\[0\\].bufList\\[0\\]\"\r\n\tb.buffers[index1+1].bufList[index1] = b.buffers[index1+1].bufList[index1][1:2:3] // ERROR \"ignoring self-assignment to b.buffers\\[index1\\ \\+ 1].bufList\\[index1\\]\"\r\n\tb.buffers[1+1].bufArr[1+1] = b.buffers[1+1].bufArr[1+1][1:2]                     // ERROR \"ignoring self-assignment to b.buffers\\[1 \\+ 1\\].bufArr\\[1 \\+ 1\\]\"\r\n\t*b.buffers[1].bufPtrList[2] = (*b.buffers[1].bufPtrList[2])[1:2:3]               // ERROR \"ignoring self-assignment to \\*b.buffers\\[1\\].bufPtrList\\[2\\]\"\r\n}\r\n\r\nfunc (b *buffer) esc() { // ERROR \"leaking param content: b\"\r\n\t// None of the cases below should trigger self-assignment optimization.\r\n\r\n\t// These slice expressions contain sub-exprs that may affect memory.\r\n\tb.buf1 = b.buf1[:length(b.buf1)][1:2]\r\n\tb.buf1 = b.buf1[1:length(b.buf1)][1:2:3]\r\n\tb.buf1 = b.buf2[:][zero():length(b.buf2)][1:2]\r\n\tb.buf1 = b.buf2[zero()+1:][:][1:2:3]\r\n\r\n\t// Due to method call inside the chain, these should not be optimized.\r\n\t// The baseObject(x) returns b.getNext() node for both sides,\r\n\t// but samesafeexpr would not consider them as \"same\".\r\n\tb.getNext().buf1 = b.getNext().buf1[1:2]\r\n\tb.getNext().buf1 = b.getNext().buf1[1:2:3]\r\n\tb.getNext().buf1 = b.getNext().buf2[1:2]\r\n\tb.getNext().buf1 = b.getNext().buf2[1:2:3]\r\n\r\n\t// Different base objects.\r\n\tb.next.next.buf1 = b.next.buf1[1:2]\r\n\tb.next.next.buf1 = b.next.buf1[1:2:3]\r\n\tb.next.buf1 = b.next.next.buf2[1:2]\r\n\tb.next.buf1 = b.next.next.buf2[1:2:3]\r\n\tb.bufList[0] = b.bufArr[0][1:2]\r\n\r\n\t// Different indexes are not permitted inside base objects.\r\n\tindex1 := 10\r\n\tindex2 := index1\r\n\tb.buffers[0].bufList[0] = b.buffers[1].bufList[0][1:2]\r\n\tb.buffers[index1].bufList[index1] = b.buffers[index2].bufList[index1][1:2:3]\r\n\tb.buffers[1+1].bufArr[1+1] = b.buffers[1+0].bufArr[1+1][1:2]\r\n\t*b.buffers[0].bufPtrList[2] = (*b.buffers[1].bufPtrList[2])[1:2:3]\r\n\tb.buffers[index1+1].bufList[index1] = b.buffers[index1+2].bufList[index1][1:2:3]\r\n}\r\n\r\nfunc (b *buffer) sanity1() { // ERROR \"leaking param content: b\"\r\n\tb.next.buf1 = b.next.buf2[:] // ERROR \"ignoring self-assignment to b.next.buf1\"\r\n\tsink = b.next.buf1           // ERROR \"b.next.buf1 escapes to heap\"\r\n\tsink = b.next.buf2           // ERROR \"b.next.buf2 escapes to heap\"\r\n}\r\n\r\nfunc (b *buffer) sanity2() { // ERROR \"b does not escape\"\r\n\tb.bufList = b.bufList[:len(b.bufList)-1] // ERROR \"ignoring self-assignment to b.bufList\"\r\n}\r\n```\r\n"},"comment":{"url":"https://api.github.com/repos/golang/go/issues/comments/423123973","html_url":"https://github.com/golang/go/issues/27772#issuecomment-423123973","issue_url":"https://api.github.com/repos/golang/go/issues/27772","id":423123973,"node_id":"MDEyOklzc3VlQ29tbWVudDQyMzEyMzk3Mw==","user":{"login":"gopherbot","id":8566911,"node_id":"MDQ6VXNlcjg1NjY5MTE=","avatar_url":"https://avatars1.githubusercontent.com/u/8566911?v=4","gravatar_id":"","url":"https://api.github.com/users/gopherbot","html_url":"https://github.com/gopherbot","followers_url":"https://api.github.com/users/gopherbot/followers","following_url":"https://api.github.com/users/gopherbot/following{/other_user}","gists_url":"https://api.github.com/users/gopherbot/gists{/gist_id}","starred_url":"https://api.github.com/users/gopherbot/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gopherbot/subscriptions","organizations_url":"https://api.github.com/users/gopherbot/orgs","repos_url":"https://api.github.com/users/gopherbot/repos","events_url":"https://api.github.com/users/gopherbot/events{/privacy}","received_events_url":"https://api.github.com/users/gopherbot/received_events","type":"User","site_admin":false},"created_at":"2018-09-20T10:05:07Z","updated_at":"2018-09-20T10:05:07Z","author_association":"NONE","body":"Change https://golang.org/cl/136496 mentions this issue: `cmd/compile/internal/gc: generalize self-assignment`"}}
{"actor":{"display_login":"gopherbot"}}