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: "add some generic composite type optimizations" only partially eliminates addrtaken parameter #26407

Closed
heschik opened this issue Jul 16, 2018 · 4 comments

Comments

Projects
None yet
4 participants
@heschik
Copy link
Contributor

commented Jul 16, 2018

In the following code:

package main

type holder struct {
	seq []string
}

func inlineme(h *holder, DELETEME []string) {
	if &DELETEME == &h.seq {
		return
	}
	h.seq = DELETEME
}

//go:noinline
func intermediate(h *holder, seq []string) {
	inlineme(h, seq)
}

func main() {
	var seq []string
	var h holder
	intermediate(&h, seq)
}

inlineme is unusual in that it takes the address of a parameter, but that parameter doesn't actually escape. When inlineme is inlined, DELETEME is almost entirely eliminated from the resulting code, but its address is still taken and it still exists in the function. I haven't followed step by step, but when the stack maps are eventually generated, they still mark DELETEME as live, even though it's never zeroed. If you happen to grow the stack while inlineme is on it, it reads stack garbage and throws.

This bisects down to CL 106495 (commit f31a18d). I imagine the problem will be something like the address operation causing the variable to be kept in func.Dcl, and then the liveness analysis keeps it live forever because it's a parameter. Or something like that.

This was reduced from a test that calls https://github.com/pmezard/go-difflib/blob/792786c7400a136282c1664665ae0a8db921c6c2/difflib/difflib.go#L115, if it matters, but you need a fairly large test with a fairly large stack to reproduce it so I just eyeballed the SSA dump. Not sure how to turn it into a clean test case.

cc @randall77, @TocarIP, @mundaym

@heschik heschik added this to the Go1.11 milestone Jul 16, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

@mundaym mundaym self-assigned this Jul 17, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Jul 17, 2018

Change https://golang.org/cl/124335 mentions this issue: cmd/compile: keep autos if their address reaches a block control

@mundaym

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

Thanks for the report @heschik. I think CL 124335 should fix the issue, can you try it on your original test case?

I'm not sure how to test this one. This function shows the same issue, y is kept and its address used in the comparison but it is not cleared:

//go:noinline
func incorrect(x **int) *int {
	var y *int // not cleared
	if x == &y {
		panic("not possible")
	}
	return *x
}

I thought I might be able to get the GC to panic if I poisoned the stack beforehand and then called runtime.GC(), but I haven't managed to make it work yet. Anyone have any ideas?

Any test for this is likely to be fragile since we might just optimize away the comparison in the future (any pointer coming from outside the function cannot be equal to the address of a local variable).

@mundaym mundaym added NeedsFix and removed NeedsInvestigation labels Jul 17, 2018

@mundaym

This comment has been minimized.

Copy link
Member

commented Jul 17, 2018

Got a test working.

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.