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

Jump to bottom

cmd/compile: dead pointers kept alive across loop iterations #18336

Closed
mdempsky opened this issue Dec 15, 2016 · 6 comments
Closed

cmd/compile: dead pointers kept alive across loop iterations #18336

mdempsky opened this issue Dec 15, 2016 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.12

Comments

@mdempsky
Copy link
Member

mdempsky commented Dec 15, 2016 •
edited

A little while back we noticed that 01bf5cc caused an internal test to start failing due to allocating too much memory. Simplifying, the test is basically:

var v reflect.Value
for m := in.pop(); m != nil; m = in.pop() {
	v = reflect.ValueOf(m)
	// ... use v ...

	// Clear out reference to m so it doesn't stay alive during in.pop().
	v = reflect.Value{}
}

This works as intended when reflect.ValueOf is not inlined. But 01bf5cc happened to cause reflect.ValueOf to become eligilble for inlining, and suddenly m was being kept alive during the in.pop() calls.

I've produced two partially minimized repros:

One with //go:noinline that pases: https://play.golang.org/p/Bu1qLfD60c

One without that fails: https://play.golang.org/p/iTNJxtmiSN

The same problem exists in 1.7.3, so targeting for 1.9.
@mdempsky mdempsky added this to the Go1.9 milestone Dec 15, 2016
@bradfitz
Copy link
Contributor

/cc @randall77

@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 15, 2016
@mdempsky
Copy link
Member Author

mdempsky commented Dec 15, 2016 •
edited

Experimenting further, the issue can be reproduced even without inlining:

for m := in.pop(); m != nil; m = in.pop() {
        i := m
        _ = &i  // dummy statement that takes i's address, but doesn't escape it
}

Here i is being kept alive during the m = in.pop() PostStmt clause.

Of course, in this case you could add i = nil to forcefully clobber the variable. That's not an option with inlined variables though.

Perhaps we need to emit VARKILL instructions for non-escaping Addrtaken variables when their innermost enclosing for block (if any) ends?

We can't emit them at just the innermost block, because that would break:

var p **int
if x {
        q := new(int)
        p = &q
}
use(p)  // q needs to still be alive here

@gopherbot
Copy link

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

@mdempsky mdempsky changed the title cmd/compile: dead pointers being kept alive due to inlining cmd/compile: dead pointers kept alive across loop iterations Dec 16, 2016
@mdempsky mdempsky mentioned this issue Feb 14, 2017
Closed
@josharian
Copy link
Contributor

josharian commented Mar 3, 2017 •
edited

Finding just the right spot for a VARKILL here seems like @dr2chase's cup o' tea.

@josharian josharian mentioned this issue Mar 9, 2017
Closed
@josharian josharian mentioned this issue May 22, 2017
Closed
@mdempsky mdempsky modified the milestones: Go1.10, Go1.9 Jul 17, 2017
@mdempsky mdempsky modified the milestones: Go1.10, Go1.11 Nov 30, 2017
@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Go1.12 Jun 30, 2018
@randall77
Copy link
Contributor

This should now be fixed due to stack tracing (CLs https://go-review.googlesource.com/c/go/+/134155 and https://go-review.googlesource.com/c/go/+/134156)

@gopherbot
Copy link

Change https://golang.org/cl/141117 mentions this issue: cmd/compile: remove some inl budget hacks

gopherbot pushed a commit that referenced this issue Oct 10, 2018
@josharian
cmd/compile: remove some inl budget hacks
555d8c4 
Prior to stack tracing, inlining could cause
dead pointers to be kept alive in some loops.
See #18336 and CL 31674.

The adjustment removed by this change preserved the inlining status quo
in the face of Node structure changes, to avoid creating new problems.
Now that stack tracing provides precision, these hacks can be removed.

Of course, our inlining code model is already hacky (#17566),
but at least now there will be fewer epicyclical hacks.

Newly inline-able functions in std cmd as a result of this change:

hash/adler32/adler32.go:65:6: can inline (*digest).UnmarshalBinary
hash/fnv/fnv.go:281:6: can inline (*sum32).UnmarshalBinary
hash/fnv/fnv.go:292:6: can inline (*sum32a).UnmarshalBinary
reflect/value.go:1298:6: can inline Value.OverflowComplex
compress/bzip2/bit_reader.go:25:6: can inline newBitReader
encoding/xml/xml.go:365:6: can inline (*Decoder).switchToReader
vendor/golang_org/x/crypto/cryptobyte/builder.go:77:6: can inline (*Builder).AddUint16
crypto/x509/x509.go:1851:58: can inline buildExtensions.func2.1.1
crypto/x509/x509.go:1871:58: can inline buildExtensions.func2.3.1
crypto/x509/x509.go:1883:58: can inline buildExtensions.func2.4.1
cmd/vet/internal/cfg/builder.go:463:6: can inline (*builder).labeledBlock
crypto/tls/handshake_messages.go:1450:6: can inline (*newSessionTicketMsg).marshal
crypto/tls/handshake_server.go:769:6: can inline (*serverHandshakeState).clientHelloInfo
crypto/tls/handshake_messages.go:1171:6: can inline (*nextProtoMsg).unmarshal
cmd/link/internal/amd64/obj.go:40:6: can inline Init
cmd/link/internal/ppc64/obj.go:40:6: can inline Init
net/http/httputil/persist.go:54:6: can inline NewServerConn
net/http/fcgi/child.go:83:6: can inline newResponse
cmd/compile/internal/ssa/poset.go:245:6: can inline (*poset).newnode

Change-Id: I19e8e383a6273849673d35189a9358870665f82f
Reviewed-on: https://go-review.googlesource.com/c/141117
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ilya Tocar <ilya.tocar@intel.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@golang golang locked and limited conversation to collaborators Oct 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
6 participants
@bradfitz @mdempsky @josharian @ianlancetaylor @randall77 @gopherbot