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: -d=checkptr should provide more details when reporting about conversions that straddle multiple allocations #40639

Open
seebs opened this issue Aug 7, 2020 · 8 comments
Assignees
Milestone

Comments

@seebs
Copy link
Contributor

@seebs seebs commented Aug 7, 2020

What version of Go are you using (go version)?

$ go version
1.14

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.)

@ianlancetaylor ianlancetaylor changed the title checkptr should say what rule it thinks you're breaking cmd/compile: -d=checkptr should say what rule it thinks you're breaking Aug 7, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 7, 2020

@ianlancetaylor ianlancetaylor added this to the Backlog milestone Aug 7, 2020
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Aug 7, 2020

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 (*[big]T)(p)[:] and hint the user should add len/cap to the slice?

@seebs
Copy link
Contributor Author

@seebs seebs commented Aug 7, 2020

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 c.array() is a []uint16 of length n, and if i use [:n:n] in the slice, it works fine and checkptr doesn't fuss. but since i know that it's a length n slice of uint16, it didn't occur to me to worry about the theoretical bounds of the slice, until someone pointed it out.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Aug 7, 2020

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".

Okay. I believe that's what 1f231d7 does. After that CL, checkptr reports unique error messages for each case it checks.

something like "converted pointer points to 8192 bytes, base pointer is 2048 bytes before end of allocation" might be helpful too.

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:

checkptr: converted pointer straddles multiple allocations (points to 2048-byte allocation, but [65536]uint16 is 131072 bytes)

Open to suggestions though.

@mdempsky mdempsky changed the title cmd/compile: -d=checkptr should say what rule it thinks you're breaking cmd/compile: -d=checkptr should provide more details when reporting about conversions that straddle multiple allocations Aug 7, 2020
@mdempsky mdempsky self-assigned this Aug 7, 2020
@seebs
Copy link
Contributor Author

@seebs seebs commented Aug 8, 2020

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 b [65536]byte, and I obtain a poiter to &b[65530], and cast it to *[16]byte. That's the case where the wording seems hard to me; "points to 16-byte allocation, but [65536]byte is 65536 bytes" would be slightly confusing.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Aug 8, 2020

Say I have b [65536]byte, and I obtain a poiter to &b[65530], and cast it to *[16]byte.

In this case, my proposed wording would be: points to 6-byte allocation, but [16]byte is 16 bytes.

@randall77
Copy link
Contributor

@randall77 randall77 commented Aug 8, 2020

Can we do points to byte offset 65530 in a 65536-byte allocation, but [16]byte is 16 bytes.
(Including the offset only if it is nonzero.)

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Aug 8, 2020

Yeah that seems doable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.