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: missed bounds check elimination when paired slices are resliced #70062

Open
FiloSottile opened this issue Oct 26, 2024 · 4 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@FiloSottile
Copy link
Contributor

go version devel go1.24-140308837f Mon Oct 21 15:30:47 2024 +0200 darwin/arm64

When the lengths of two slices are "paired" (dst = dst[:len(src)]), the compiler is smart enough to apply ifs to both, but if you reslice them the pairing is lost.

func foo(src, dst []byte) {
	dst = dst[:len(src)]
	for len(src) >= 50 {
		_ = (*[50]byte)(src) // no bounds check
		_ = (*[50]byte)(dst) // no bounds check
	}
}

func bar(src, dst []byte) {
	dst = dst[:len(src)]
	for len(src) >= 50 {
		src = src[50:] // no bounds check
		dst = dst[50:] // bounds check!
	}
}
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Oct 26, 2024
@randall77
Copy link
Contributor

I'm not sure how we'd do this.

In the former case, there's no modifications to src or dst (after the first slicing), so when we conclude that len(src)==len(dst) after the first slicing, it continues to hold throughout the rest of the function.

In the latter case, src and dst are modified in the loop. So we not only need to know that len(src)==len(dst) at the start, but that src=src[50:];dst=dst[50:] maintains that invariant. And that we never use that invariant between those two statements (where it would not hold).

We don't have any mechanism for that kind of logic in the compiler right now. I'm not sure how we would do it.

(That first function doesn't make much sense to me. An infinite loop is probably not the intent.)

@FiloSottile
Copy link
Contributor Author

I don't always know what's doable and what is not, so when in doubt I file :)

The first function was just to show there is some awareness of len(src)==len(dst). Should have been an if not a for.

@randall77 randall77 added this to the Unplanned milestone Oct 26, 2024
@zigo101
Copy link

zigo101 commented Oct 27, 2024

When for is replaced with if, then the check is removed.

[edit]: it is indeed a problem in reality:

func bar(src, dst []byte) {
	dst = dst[:len(src)]
	for len(src) >= 2 {
		dst[1] = // Found IsInBounds
			src[0]
		dst[0] = src[1]
		src = src[2:]
		dst = dst[2:]
	}
}

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests

6 participants