cmd/compile: slightly unclear heap escape message (with -m) #16300

cespare · 2016-07-08T17:21:30Z

Tested with linux/amd64, go 1.6.2 and tip.

Consider the following function and -gcflags -m output:

219|func readString(r io.Reader, buf []byte) ([]byte, error) {
220|    var scratch [4]byte
221|    if _, err := io.ReadFull(r, scratch[:]); err != nil {
222|        return nil, err
223|    }
224|    n := int(binary.BigEndian.Uint32(scratch[:]))
225|    if n > maxStringLength {
226|        return nil, ErrStringTooBig
227|    }
228|    if n <= cap(buf) {
229|        buf = buf[:n]
230|    } else {
231|        buf = make([]byte, n)
232|    }
233|    if _, err := io.ReadFull(r, buf); err != nil {
234|        return nil, err
235|    }
236|    return buf, nil
237|}

x.go:219: leaking param: r
x.go:220: moved to heap: scratch                <-------
x.go:221: scratch escapes to heap
x.go:231: make([]byte, n) escapes to heap
x.go:219: leaking param: buf
x.go:231: make([]byte, n) escapes to heap
x.go:231: make([]byte, n) escapes to heap
x.go:224: readString scratch does not escape    <-------

At first glance the two indicated lines seem to contradict each other. After consulting with some other folks I think that the last line ("x.go:224: readString scratch does not escape") means that the function call using scratch on line 224 does not itself cause scratch to escape, even though scratch does escape. Rather, it is the usage on line 221 that causes scratch to escape.

Perhaps the message could be reworded a bit.

/cc @dr2chase, I think?

The text was updated successfully, but these errors were encountered:

cespare · 2016-07-08T23:33:04Z

By the way, with tip, the -m -m output is

x.go:219: leaking param: r
x.go:219:  from r (passed to function[unknown]) at x.go:219
x.go:221: scratch escapes to heap
x.go:221:  from scratch[:] (passed to function[unknown]) at x.go:221
x.go:220: moved to heap: scratch
x.go:231: make([]byte, n) escapes to heap
x.go:231:  from make([]byte, n) (too large for stack) at x.go:231
x.go:219: leaking param: buf
x.go:219:  from buf (passed to function[unknown]) at x.go:219
x.go:224: readString scratch does not escape

rsc · 2016-10-20T14:19:17Z

Yes, the explanation in the initial report is correct. The "readString" is a prefix to say which function is being reported. All the messages are supposed to have the prefix. Or else none should, but probably all.

dr2chase · 2016-10-20T14:47:56Z

Apart from the readString prefix, should this be suffixed with " here" or " at this call"?

dr2chase · 2016-10-21T19:35:06Z

Supposing the "does not escape" messages were removed, thus no-news-is-good-news, instead of the mixed bag that we have now?

mrjrieke · 2016-10-30T21:56:51Z

What is the compiler trying to say here?

Is it saying, "I thought scratch was escaping to heap, but after closer analysis, now I see that it isn't?", is it trying to say, "scratch is not escaping to the heap at this particular point(i.e. 2nd reference escape)?", or is it saying something else entirely?

dr2chase · 2016-10-31T14:09:28Z

I think the compiler is failing to admit that it was previously in error, and there is no way to call back that earlier false claim. It merely appears "after" because of an artifact of the line numbers at which it is discovered.

Once something is determined to escape, it stays escaped.

mrjrieke · 2016-10-31T16:51:20Z

Thanks @dr2chase. That's too bad. One of the things I wish go could do better, function inline variable escape analysis.

I won't pretend to understand the internals of this, but it seems if go escape analyzer could inline all inlineable functions first, then it might have better handle on which variables have potential for stack scope.

josharian · 2016-10-31T16:55:46Z

@mrjrieke actually, inlining does occur very early, before escape analysis. And that is it's own source of missed optimization opportunities (#17566).

mrjrieke · 2016-10-31T17:04:52Z

Thanks, @josharian, let me take a look at that. Maybe it'll answer some of my puzzlings.

cherrymui · 2017-11-30T16:24:03Z

When slicing an array, the compiler inserts an implicit OADDR operator, so scratch[:] is actually

.   .   .   .   .   SLICEARR l(11) tc(1) SLICE-[]byte
.   .   .   .   .   .   ADDR l(11) tc(1) implicit(true) PTR64-*[4]byte
.   .   .   .   .   .   .   NAME-p.scratch a(true) g(5) l(10) x(0) class(PAUTO) f(1) tc(1) addrtaken assigned used ARRAY-[4]byte

For the two call sites,
x.go:221: scratch escapes to heap (io.ReadFull(r, scratch[:]))
and
x.go:224: readString scratch does not escape (binary.BigEndian.Uint32(scratch[:])),
the OADDR node is actually two different nodes. In both cases, the compiler prints the diagnostics with the implicit OADDR node, not the ONAME node. The printer, on the other hand, does not print implicit OADDR operator (https://go.googlesource.com/go/+/4435fcfd6c3e9751b470809307b4afc7c1769098/src/cmd/compile/internal/gc/fmt.go#1200). This causes the confusion.

One possible fix: do not print does not escape message for implicit node. At least for OADDR, the underlying non-implicit node should be tracked, and there will be a message printed if the underlying node does indeed not escape.

ianlancetaylor · 2017-12-06T18:21:20Z

@dr2chase @cherrymui Is this going to be fixed for 1.10? If not lets roll the milestone forward.

dr2chase · 2017-12-06T19:04:12Z

This "One possible fix: do not print does not escape message for implicit node" turns out to need more help; alone, it causes many apparently useful and correct messages to disappear. There's loads of regressions in the escape*.go tests.

mdempsky · 2019-04-01T20:42:19Z

I think this is fixed by https://go-review.googlesource.com/c/go/+/170319. With that CL, these messages are suppressed:

x.go:221: scratch escapes to heap
x.go:224: readString scratch does not escape

N.B., that this message (which is the most important one) is kept:

x.go:220: moved to heap: scratch

gopherbot · 2019-04-01T20:49:16Z

Change https://golang.org/cl/170319 mentions this issue: cmd/compile: skip escape analysis diagnostics for OADDR

For most nodes (e.g., OPTRLIT, OMAKESLICE, OCONVIFACE), escape analysis prints "escapes to heap" or "does not escape" to indicate whether that node's allocation can be heap or stack allocated. These messages are also emitted for OADDR, even though OADDR does not actually allocate anything itself. Moreover, it's redundant because escape analysis already prints "moved to heap" diagnostics when an OADDR node like "&x" causes x to require heap allocation. Because OADDR nodes don't allocate memory, my escape analysis rewrite doesn't naturally emit the "escapes to heap" / "does not escape" diagnostics for them. It's also non-trivial to replicate the exact semantics esc.go uses for OADDR. Since there are so many of these messages, I'm disabling them in this CL by themselves. I modified esc.go to suppress the Warnl calls without any other behavior changes, and then used a shell script to automatically remove any ERROR messages mentioned by run.go in "missing error" or "no match for" lines. Fixes golang#16300. Updates golang#23109. Change-Id: I3993e2743c3ff83ccd0893f4e73b366ff8871a57

ianlancetaylor added this to the Go1.8Maybe milestone Jul 8, 2016

ianlancetaylor assigned dr2chase Jul 8, 2016

quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 10, 2016

rsc modified the milestones: Go1.9, Go1.8Maybe Nov 11, 2016

josharian modified the milestones: Go1.9Maybe, Go1.9 Jun 25, 2017

bradfitz modified the milestones: Go1.9Maybe, Go1.10 Jul 20, 2017

dr2chase modified the milestones: Go1.10, Go1.11 Dec 6, 2017

dr2chase modified the milestones: Go1.11, Go1.12 Jun 26, 2018

randall77 modified the milestones: Go1.12, Unplanned Nov 27, 2018

gopherbot closed this as completed in abefcac Apr 2, 2019

golang locked and limited conversation to collaborators Apr 1, 2020

gopherbot added the FrozenDueToAge label Apr 1, 2020

rsc unassigned dr2chase Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/compile: slightly unclear heap escape message (with -m) #16300

cmd/compile: slightly unclear heap escape message (with -m) #16300

cespare commented Jul 8, 2016

cespare commented Jul 8, 2016 •

edited

rsc commented Oct 20, 2016

dr2chase commented Oct 20, 2016

dr2chase commented Oct 21, 2016

mrjrieke commented Oct 30, 2016

dr2chase commented Oct 31, 2016

mrjrieke commented Oct 31, 2016

josharian commented Oct 31, 2016

mrjrieke commented Oct 31, 2016

cherrymui commented Nov 30, 2017

ianlancetaylor commented Dec 6, 2017

dr2chase commented Dec 6, 2017 •

edited

mdempsky commented Apr 1, 2019 •

edited

gopherbot commented Apr 1, 2019

cmd/compile: slightly unclear heap escape message (with -m) #16300

cmd/compile: slightly unclear heap escape message (with -m) #16300

Comments

cespare commented Jul 8, 2016

cespare commented Jul 8, 2016 • edited

rsc commented Oct 20, 2016

dr2chase commented Oct 20, 2016

dr2chase commented Oct 21, 2016

mrjrieke commented Oct 30, 2016

dr2chase commented Oct 31, 2016

mrjrieke commented Oct 31, 2016

josharian commented Oct 31, 2016

mrjrieke commented Oct 31, 2016

cherrymui commented Nov 30, 2017

ianlancetaylor commented Dec 6, 2017

dr2chase commented Dec 6, 2017 • edited

mdempsky commented Apr 1, 2019 • edited

gopherbot commented Apr 1, 2019

cespare commented Jul 8, 2016 •

edited

dr2chase commented Dec 6, 2017 •

edited

mdempsky commented Apr 1, 2019 •

edited