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
heschi opened this issue Jul 16, 2018 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@heschi
Copy link
Contributor

heschi 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

@heschi heschi added this to the Go1.11 milestone Jul 16, 2018
@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 16, 2018
@ianlancetaylor
Copy link
Contributor

CC @dr2chase

@mundaym mundaym self-assigned this Jul 17, 2018
@gopherbot
Copy link

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

@mundaym
Copy link
Member

mundaym 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 The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 17, 2018
@mundaym
Copy link
Member

mundaym commented Jul 17, 2018

Got a test working.

@golang golang locked and limited conversation to collaborators Jul 17, 2019
@rsc rsc unassigned mundaym Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

4 participants