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

runtime: emit better errors from checkptr #37488

Closed
dominikh opened this issue Feb 27, 2020 · 5 comments
Closed

runtime: emit better errors from checkptr #37488

dominikh opened this issue Feb 27, 2020 · 5 comments
Milestone

Comments

@dominikh
Copy link
Member

@dominikh dominikh commented Feb 27, 2020

Currently, checkptr checks for four different kinds of mistakes; two of them concerning alignment, two of them concerning pointer arithmetic. However, it only emits two different kinds of errors, one per class of mistakes. It would be helpful to know which specific kind of mistake I made. Did I ignore alignment, or did I straddle multiple objects?

Furthermore, the errors themselves can be confusing. unsafe pointer conversion will point to a line of code that uses unsafe.Pointer conversion; it is not immediately obvious that these two uses of the word "unsafe" mean different things – checkptr really complains about potentially broken pointer conversions.

@josharian
Copy link
Contributor

@josharian josharian commented Feb 27, 2020

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 27, 2020

All good points. I'm open to improving the error messages.

Some suggestions:

checkptr: misaligned pointer conversion
checkptr: converted pointer straddles multiple allocations
checkptr: pointer arithmetic computed bad pointer value
checkptr: pointer arithmetic result points to invalid allocation

I'm also wondering if "checkptr:" should change to "unsafe:" for consistency with warnings about packages runtime and sync, or if it should stay "checkptr" to make the compiler flag connection clearer.

@cespare
Copy link
Contributor

@cespare cespare commented Feb 27, 2020

Related to #37298.

@dominikh
Copy link
Member Author

@dominikh dominikh commented Feb 27, 2020

I'm torn between the two prefixes, both have their advantage; I'm slightly leaning towards "checkptr:", because then people might not wonder as much why unsafe reports errors when using the race detector and not otherwise. Definitely makes it easier to google for "checkptr".

Your suggestions definitely improve upon the current messages, and we'll have a whole cycle to bikeshed them to death :-)

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 11, 2020

Change https://golang.org/cl/223037 mentions this issue: runtime: emit more specific errors from checkptr

@gopherbot gopherbot closed this in 1f231d7 Mar 12, 2020
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
6 participants
You can’t perform that action at this time.