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/compile: slightly unclear heap escape message (with -m) #16300

Closed
cespare opened this issue Jul 8, 2016 · 14 comments

Comments

@cespare
Copy link
Contributor

commented Jul 8, 2016

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?

@cespare

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2016

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

@quentinmit quentinmit added the NeedsFix label Oct 10, 2016

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2016

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

@dr2chase

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2016

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

@mrjrieke

This comment has been minimized.

Copy link

commented Oct 30, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

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

This comment has been minimized.

Copy link

commented Oct 31, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

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

@mrjrieke

This comment has been minimized.

Copy link

commented Oct 31, 2016

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

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

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

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

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2017

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

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

@dr2chase

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2017

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.

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

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

@mdempsky

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

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

This comment has been minimized.

Copy link

commented Apr 1, 2019

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

mdempsky added a commit to mdempsky/go that referenced this issue Apr 1, 2019
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

@gopherbot gopherbot closed this in abefcac Apr 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.