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: GOEXPERIMENT=clobberdead is broken #27326

Closed
cherrymui opened this issue Aug 29, 2018 · 5 comments

Comments

Projects
None yet
4 participants
@cherrymui
Copy link
Contributor

commented Aug 29, 2018

What version of Go are you using (go version)?

tip (618bfb2)

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

darwin/amd64

What did you do?

$ GOEXPERIMENT=clobberdead ./all.bash 
Building Go cmd/dist using /Users/cherryyz/src/go1.8.
Building Go toolchain1 using /Users/cherryyz/src/go1.8.
Building Go bootstrap cmd/go (go_bootstrap) using Go toolchain1.
Building Go toolchain2 using go_bootstrap and Go toolchain1.
go tool dist: FAILED: /Users/cherryyz/src/go-tip/pkg/tool/darwin_amd64/go_bootstrap install -gcflags=all= -ldflags=all= -i cmd/asm cmd/cgo cmd/compile cmd/link: exit status 2

Not sure if it catches a real bug, or the experiment itself is broken. I may take a look tomorrow.

@cherrymui cherrymui added this to the Go1.12 milestone Aug 29, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Aug 29, 2018

Change https://golang.org/cl/131956 mentions this issue: cmd/compile: only clobber dead slots at call sites

@gopherbot

This comment has been minimized.

Copy link

commented Aug 29, 2018

Change https://golang.org/cl/131957 mentions this issue: cmd/compile: don't clobber dead slots in runtime.wbBufFlush

@cherrymui

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2018

The main thing is that we now have many more safepoints, at nearly all instructions. Clobbering at all these safepoints currently doesn't work. Maybe the stack maps at non-call safepoints are still imprecise, or some other reason. I haven't investigated. CL 131956 disables non-call safepoints for clobbering.

With the two CLs above, I got it mostly working on Linux AMD64. There are a few tests failing, that are incompatible with the experiment, for example, explicitly testing register maps. And when bootstrapping cmd/go complains about some packages are stall, which I have no idea how to debug, so I just skipped the check (by setting a VERSION). Besides those, it works fine.

On Darwin AMD64, it still doesn't work. Still looking.

gopherbot pushed a commit that referenced this issue Aug 30, 2018

cmd/compile: only clobber dead slots at call sites
We now have safepoints at nearly all the instructions. When
GOEXPERIMENT=clobberdead is on, it inserts clobbers nearly at
every instruction. Currently this doesn't work. (Maybe the stack
maps at non-call safepoints are still imprecise. I haven't
investigated.) For now, only use call-based safepoints if the
experiment is on.

Updates #27326.

Change-Id: I72cda9b422d9637cc5738e681502035af7a5c02d
Reviewed-on: https://go-review.googlesource.com/131956
Reviewed-by: Keith Randall <khr@golang.org>

gopherbot pushed a commit that referenced this issue Aug 30, 2018

cmd/compile: don't clobber dead slots in runtime.wbBufFlush
runtime.wbBufFlush must not modify its arguments, because the
argument slots are also used as spill slots in runtime.gcWriteBarrier.
So, GOEXPERIMENT=clobberdead must not clobber them.

Updates #27326.

Change-Id: Id02bb22a45201eecee748d89e7bdb3df7e4940e4
Reviewed-on: https://go-review.googlesource.com/131957
Reviewed-by: Keith Randall <khr@golang.org>
@randall77

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

Now that we have stack objects, there's the additional complication that we can't clobber stack objects (all address-taken local variables) because plive doesn't know for sure that they are dead anymore.

clobberdead was originally intended to test all the weird ambiguously live logic. Maybe now that all that logic is gone, we can retire this mode.

@gopherbot

This comment has been minimized.

Copy link

commented Nov 26, 2018

Change https://golang.org/cl/151317 mentions this issue: cmd/compile: remove CLOBBERDEAD experiment

@gopherbot gopherbot closed this in 57c8eb9 Nov 26, 2018

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