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: type safety failure in generic ssa due to representing integer signedness conversions by OpCopy #37753
Comments
Looking at the commit in question I can see that a workaround in Gonum for this is
but the error is something that probably should be warned about at the very least, and it's odd that it affects the unsafe-based code, but not the (at the apparently relevant point) reflect-based code since they both have |
Thanks for catching this early, @kortschak, and for the concise reproducer. Diagnosis: On our way into SSA form, there's an OCONV node converting I'll back out that part of the compiler change for now. (CL coming probably tomorrow, I have to sign off now.) But this also points to another source of type-unsafety during the generic SSA stages, which @randall77 and @cherrymui and I have bumped up against a few times this cycle. We might want to try to restore some type safety there, at which point might be able to restore the optimization. |
Simpler reproducer (and now I'm signing off!): package main
//go:noinline
func f(a, b uint) int {
return int(a-b) / 8
}
func main() {
if x := f(1, 2); x != 0 {
panic(x)
}
} |
Change https://golang.org/cl/222620 mentions this issue: |
CL 212777 added a check to isNonNegative to return true for unsigned values. However, the SSA backend isn't type safe enough for that to be sound. The other checks in isNonNegative look only at the pattern of bits. Remove the type-based check. Updates #37753 Change-Id: I059d0e86353453133f2a160dce53af299f42e533 Reviewed-on: https://go-review.googlesource.com/c/go/+/222620 Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
We are (I hope!) back to generating correct code again. The remaining question is around how to make generic ssa type-correct again. Two options I see: Introduce a conversion op specifically for converting beteeen signed and unsigned ints. (2) Changing copyelim to not eliminate copies with different types. |
Yeah, I think we need some sort of reinterpret/cast opcode in the machine-independent form. I'd rather not foist this task on OpCopy. If anything, if v.Op == OpCopy then we should have v.Type == v.Args[0].Type. Similar for OpPhi. It might be kinda ugly to get there though. |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
No
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
Using the Go built at or after ea3bfba run the code at https://play.golang.org/p/ah0Rl1b03Li
What did you expect to see?
What did you see instead?
Additional information
This code is used in the Gonum matrix package to detect parameter shadowing, for example here, and is the subject of the (now quite old) #12445. It would be wonderful to have some certainty on this.
/cc @josharian
The text was updated successfully, but these errors were encountered: