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

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

cmd/compile: GOEXPERIMENT=clobberdead is broken #27326

cherrymui opened this issue Aug 29, 2018 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.12

Comments

@cherrymui
Copy link
Member

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
Copy link

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

@gopherbot
Copy link

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

@cherrymui
Copy link
Member Author

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
@cherrymui
cmd/compile: only clobber dead slots at call sites
c854572 
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
@cherrymui
cmd/compile: don't clobber dead slots in runtime.wbBufFlush
360771e 
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>
@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 30, 2018
@randall77
Copy link
Contributor

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
Copy link

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

@golang golang locked and limited conversation to collaborators Nov 26, 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.
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
4 participants
@FiloSottile @randall77 @gopherbot @cherrymui