[go] src/unsafe: add comment on alignment assumptions for misaligned accesses
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
src/unsafe: add comment on alignment assumptions for misaligned accesses
Aims to clarify the assumptions the compiler makes on misaligned accesses wrt unsafe code.
Following conversations with Alex Vlasov and https://github.com/golang/go/issues/76919#issuecomment-3675508605 -- it seems that the compiler always assumes that pointers are aligned to their types.
Change diff
diff --git a/src/unsafe/unsafe.go b/src/unsafe/unsafe.go
index de9421b..0e83b31 100644
--- a/src/unsafe/unsafe.go
+++ b/src/unsafe/unsafe.go
@@ -83,6 +83,9 @@
//
// It is valid both to add and to subtract offsets from a pointer in this way.
// It is also valid to use &^ to round pointers, usually for alignment.
+// When converting the result to a pointer value p of some type *T and dereferencing,
+// p must be appropriately aligned for T. The compiler assumes
+// uintptr(p) % unsafe.Alignof(*p) == 0 for any dereference *p.
// In all cases, the result must continue to point into the original allocated object.
//
// Unlike in C, it is not valid to advance a pointer just beyond the end of
Change information
- M src/unsafe/unsafe.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot added 1 comment![]( "Open in Gerrit")
I spotted some possible problems with your PR:
1. You have a long 90 character line in the commit message body. Please add line breaks to long lines that should be wrapped. Lines in the commit message body should be wrapped at ~76 characters unless needed for things like URLs or tables. (Note: GitHub might render long lines as soft-wrapped, so double-check in the Gerrit commit message shown above.)
Please address any problems by updating the GitHub PR.
When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above. These findings are based on heuristics; if a finding does not apply, briefly reply here saying so.
To update the commit title or commit message body shown here in Gerrit, you must edit the GitHub PR title and PR description (the first comment) in the GitHub web interface using the 'Edit' button or 'Edit' menu entry there. Note: pushing a new commit to the PR will not automatically update the commit message used by Gerrit.
For more details, see:
- [how to update commit messages](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message) for PRs imported into Gerrit.
- the Go project's [conventions for commit messages](https://go.dev/doc/contribute#commit_messages) that you should follow.
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevaundray Wedderburn (Gerrit)
Kevaundray Wedderburn added 1 comment![]( "Open in Gerrit")
I spotted some possible problems with your PR:
1. You have a long 90 character line in the commit message body. Please add line breaks to long lines that should be wrapped. Lines in the commit message body should be wrapped at ~76 characters unless needed for things like URLs or tables. (Note: GitHub might render long lines as soft-wrapped, so double-check in the Gerrit commit message shown above.)Please address any problems by updating the GitHub PR.
When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above. These findings are based on heuristics; if a finding does not apply, briefly reply here saying so.
To update the commit title or commit message body shown here in Gerrit, you must edit the GitHub PR title and PR description (the first comment) in the GitHub web interface using the 'Edit' button or 'Edit' menu entry there. Note: pushing a new commit to the PR will not automatically update the commit message used by Gerrit.
For more details, see:
- [how to update commit messages](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message) for PRs imported into Gerrit.
- the Go project's [conventions for commit messages](https://go.dev/doc/contribute#commit_messages) that you should follow.
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Done
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jorropo (Gerrit)
Jorropo voted and added 3 comments![]( "Open in Gerrit")
Votes added by Jorropo
| Commit-Queue | +1 |
3 comments
This seems fine to me but this wasn't thought through,
To avoid biting us in the future I think it would make more sense to remove dereferencing from the documentation.
So tell people that a `*T` merely existing needs to be aligned even tho AFAIK this is completely safe right now.
This isn't a break from how the `unsafe` package currently works, there are many in-practice completely safe things that break rules laid out in `unsafe`'s documentation.
~~I don't know on the top of my head if `checkptr` implement alignment check,~~
after checking I couldn't get it to fail on unaligned operations.
If someone were to add alignement checks to `checkptr`, I think the best option would be to do it when converting `unsafe.Pointer` → `*T` or when converting `unsafe.Pointer` → `*T` and when dereferencing `*T`, to increase the likelyhood `checkptr` would catch invalid unsafe operations.
Also see: https://github.com/golang/go/issues/37298```suggestion
Also see #37298
```
// uintptr(p) % unsafe.Alignof(*p) == 0 for any dereference *p.```suggestion
// uintptr(p) % unsafe.Alignof(*p) == 0 for any dereference p.
```
`p` is the pointer in this context, so `*p` is the value,
the pointer is what needs to be aligned not the value.
Related details
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Jorropo
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevaundray Wedderburn (Gerrit)
Kevaundray Wedderburn added 3 comments![]( "Open in Gerrit")
This seems fine to me but this wasn't thought through,
To avoid biting us in the future I think it would make more sense to remove dereferencing from the documentation.
So tell people that a `*T` merely existing needs to be aligned even tho AFAIK this is completely safe right now.This isn't a break from how the `unsafe` package currently works, there are many in-practice completely safe things that break rules laid out in `unsafe`'s documentation.
~~I don't know on the top of my head if `checkptr` implement alignment check,~~
after checking I couldn't get it to fail on unaligned operations.If someone were to add alignement checks to `checkptr`, I think the best option would be to do it when converting `unsafe.Pointer` → `*T` or when converting `unsafe.Pointer` → `*T` and when dereferencing `*T`, to increase the likelyhood `checkptr` would catch invalid unsafe operations.
this is completely safe right now.
Can you elaborate on this? ie is it safe by coincidence, but should not be done in user code (ie its invalid and there are no guarantees that it will be completely safe in a future compiler version) or is it safe now according to the language specs?
Also can we assume that the term invalid is equivalent to "undefined behaviour"?
Also see: https://github.com/golang/go/issues/37298Kevaundray Wedderburn```suggestion
Also see #37298
```
Done
// uintptr(p) % unsafe.Alignof(*p) == 0 for any dereference *p.```suggestion
// uintptr(p) % unsafe.Alignof(*p) == 0 for any dereference p.
```
`p` is the pointer in this context, so `*p` is the value,
the pointer is what needs to be aligned not the value.
Done
Related details
- Jorropo
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevaundray Wedderburn (Gerrit)
Kevaundray Wedderburn added 1 comment![]( "Open in Gerrit")
Kevaundray WedderburnThis seems fine to me but this wasn't thought through,
To avoid biting us in the future I think it would make more sense to remove dereferencing from the documentation.
So tell people that a `*T` merely existing needs to be aligned even tho AFAIK this is completely safe right now.This isn't a break from how the `unsafe` package currently works, there are many in-practice completely safe things that break rules laid out in `unsafe`'s documentation.
~~I don't know on the top of my head if `checkptr` implement alignment check,~~
after checking I couldn't get it to fail on unaligned operations.If someone were to add alignement checks to `checkptr`, I think the best option would be to do it when converting `unsafe.Pointer` → `*T` or when converting `unsafe.Pointer` → `*T` and when dereferencing `*T`, to increase the likelyhood `checkptr` would catch invalid unsafe operations.
this is completely safe right now.
Can you elaborate on this? ie is it safe by coincidence, but should not be done in user code (ie its invalid and there are no guarantees that it will be completely safe in a future compiler version) or is it safe now according to the language specs?
Also can we assume that the term invalid is equivalent to "undefined behaviour"?
on checkptr, looking at the checkPtrAlignment function in src/runtime/checkptr.go -- the alignment check only kicks in if the type contains pointers. I couldn't find the full rationale, but I'm guessing that the types without pointers don't affect GC, so misalignment is "less dangerous" at least from the runtime perspective
Related details
- Jorropo
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevaundray Wedderburn (Gerrit)
Kevaundray Wedderburn added 1 comment![]( "Open in Gerrit")
Kevaundray WedderburnThis seems fine to me but this wasn't thought through,
To avoid biting us in the future I think it would make more sense to remove dereferencing from the documentation.
So tell people that a `*T` merely existing needs to be aligned even tho AFAIK this is completely safe right now.This isn't a break from how the `unsafe` package currently works, there are many in-practice completely safe things that break rules laid out in `unsafe`'s documentation.
~~I don't know on the top of my head if `checkptr` implement alignment check,~~
after checking I couldn't get it to fail on unaligned operations.If someone were to add alignement checks to `checkptr`, I think the best option would be to do it when converting `unsafe.Pointer` → `*T` or when converting `unsafe.Pointer` → `*T` and when dereferencing `*T`, to increase the likelyhood `checkptr` would catch invalid unsafe operations.
Kevaundray Wedderburnthis is completely safe right now.
Can you elaborate on this? ie is it safe by coincidence, but should not be done in user code (ie its invalid and there are no guarantees that it will be completely safe in a future compiler version) or is it safe now according to the language specs?
Also can we assume that the term invalid is equivalent to "undefined behaviour"?
on checkptr, looking at the checkPtrAlignment function in src/runtime/checkptr.go -- the alignment check only kicks in if the type contains pointers. I couldn't find the full rationale, but I'm guessing that the types without pointers don't affect GC, so misalignment is "less dangerous" at least from the runtime perspective
Decision was made in a go runtime meeting here: https://github.com/golang/go/issues/37298#issuecomment-600275660 but seems there was no decision on whether it is allowed or not.
Liam Maxx (Gerrit)
Liam Maxx added 1 comment![]( "Open in Gerrit")
Related details
- Jorropo
- Robert Griesemer
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liam Maxx (Gerrit)
Liam Maxx added 1 comment![]( "Open in Gerrit")
Related details
- Liam Maxx
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |