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: -d=checkptr should provide more details when reporting about conversions that straddle multiple allocations #40639
Comments
CC @mdempsky |
Go 1.15 will improve the error message somewhat: 1f231d7. (Your case will now report "converted pointer straddles multiple allocations".) Does that address your concerns, or are you suggesting that checkptr should specifically recognize |
I think what I'm mostly advocating for is "at the point of deciding to report a thing, flag in some way the basis for the decision". That message would probably have communicated the issue to me (although I might have mistaken it for alignment anyway). So, the specific thing of saying that len/cap is needed might be overkill, but something like "converted pointer points to 8192 bytes, base pointer is 2048 bytes before end of allocation" might be helpful too. But mostly, things like distinguishing between "this looks to be looking outside the object the pointer was derived from" and "this is unaligned" and whatnot seem like they'd be useful. In this case, i happen to know that |
Okay. I believe that's what 1f231d7 does. After that CL, checkptr reports unique error messages for each case it checks.
We don't always know the "N bytes before end of allocation" part (e.g., the base pointer might be to a statically-allocated, package-level Go variable, or it might be into C memory), but we could certainly include additional details like that when we do know them. I think we can simplify the wording though. E.g., maybe something like:
Open to suggestions though. |
I think this is a case where more information is almost always better, and any additional insight into the violation will be valuable to the user. Say I have |
In this case, my proposed wording would be: |
Can we do |
Yeah that seems doable. |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
probably?
What operating system and processor architecture are you using (
go env
)?linux/amd64
What did you do?
dataOffset += 2 * copy((*[1 << 16]uint16)(unsafe.Pointer(&nextData[0]))[:], c.array())
(Context actually mostly irrelevant; what's important is the lack of explicit length/cap on the slice.)
What did you expect to see?
checkptr thinking i totally know what i'm doing, or telling me what's wrong
What did you see instead?
"unsafe pointer conversion"
I mentioned this in passing because I thought it was complaining that we couldn't prove that the pointer was aligned, but bcmills pointed me at the CL for having checkptr not complain about unaligned access when it's to non-pointer-typed data. Another user (hi kale!) pointed out a more subtle issue: https://go-review.googlesource.com/c/sys/+/225418
It turns out checkptr also complains about slices which lack a length/capacity. That's completely reasonable, but I didn't know it was one of the rules checkptr would enforce, and the diagnostic didn't say what it was doing.
Suggestion: checkptr diagnostics should be distinct, and say things like "potential misalignment", or "slice without length or capacity" or something else that would make it more immediately obvious what it's complaining about.
(It's right, though.)
The text was updated successfully, but these errors were encountered: