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

Jump to bottom

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
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Backlog

Comments

@dominikh
Copy link
Member

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.
@dominikh dominikh added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 27, 2020
@josharian
Copy link
Contributor

@mdempsky

@mdempsky
Copy link
Member

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 commented Feb 27, 2020

Related to #37298.

@dominikh
Copy link
Member Author

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

@bcmills bcmills mentioned this issue Mar 9, 2020
Closed
@bcmills bcmills added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Mar 9, 2020
@bcmills bcmills added this to the Backlog milestone Mar 9, 2020
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 9, 2020
@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Mar 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@mdempsky @dominikh @josharian @cespare @bcmills @gopherbot