elaborate on slice wide pointer metadata by RalfJung · Pull Request #1499 · rust-lang/reference (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
| for the metadata is determined by the type of the unsized tail: |
|---|
| * `dyn Trait` metadata is invalid if it is not a pointer to a vtable for |
| `Trait` that matches the actual dynamic trait the pointer or reference points to. |
| `Trait` that matches the actual dynamic type the pointer or reference points to. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preexisting but how can this be a requirement for wide pointers, whose data portion is allowed to dangle, I assume?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... yeah the actually implemented requirement is that the trait matches the trait given in the pointer type, i.e. a *const dyn Debug that points to a vtable for Display is UB.
@rust-lang/opsem / @RalfJung: In looking carefully at this on the lang-docs call, we realized it might be helpful, both to us and when the lang teams takes this up, if someone might be able annotate the changes here (e.g. using the GH review features), with some explanation for each of these (e.g. "this was incorrect because...", "this was already true because we had said...", "we're adding this guarantee here because...").
| * Invalid metadata in a wide reference, `Box`, or raw pointer: |
|---|
| * A reference or `Box` that is [dangling], misaligned, or points to an invalid value |
| (using the actual dynamic type of the pointee as determined by the vtable in |
| the metadata in case of dynamically sized types). |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not change anything, it clarifies what the pointed-to value is in case of e.g. &dyn Trait.
| (using the actual dynamic type of the pointee as determined by the vtable in |
|---|
| the metadata in case of dynamically sized types). |
| * Invalid metadata in a wide reference, `Box`, or raw pointer. The requirement |
| for the metadata is determined by the type of the unsized tail: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another piece of clarification, previously we were not very clear on what the metadata requirements are for types like (u32, [u32]): we spoke about "slice metadata" and the fact that this covered all types whose unsized tail is a slice was left implicit. Now it is explicit.
| * A reference or `Box` that is [dangling], misaligned, or points to an invalid value. |
|---|
| * Invalid metadata in a wide reference, `Box`, or raw pointer: |
| * `dyn Trait` metadata is invalid if it is not a pointer to a vtable for |
| `Trait` that matches the actual dynamic trait the pointer or reference points to. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"that matches the actual dynamic trait" was clearly nonsense. Also this is now moved to the "points to an invalid value" point since it's not about the metadata, it's about using the metadata to keep going recursively through the reference.
| * Slice (`[T]`) metadata is invalid if the length is not a valid `usize` |
|---|
| (i.e., it must not be read from uninitialized memory). |
| Furthermore, for wide references and `Box`, slice metadata is invalid |
| if it makes the total size of the pointed-to value bigger than `isize::MAX`. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the PR has two commits; it may help to consider them separately.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Starting a lang RFC so it's officially approved, and we can use that to update docs elsewhere following this.
@rfcbot fcp merge
Team member @scottmcm has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed.
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!
cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.
🔔 This is now entering its final comment period, as per the review above. 🔔
psst @scottmcm, I wasn't able to add the final-comment-period label, please do so.
@rustbot labels -I-lang-nominated +final-comment-period
This is now in FCP so we can unnominate.
The final comment period, with a disposition to merge, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
This will be merged soon.
psst @scottmcm, I wasn't able to add the finished-final-comment-period label, please do so.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks like FCP has finished.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
tgross35 added a commit to tgross35/rust that referenced this pull request
tgross35 added a commit to tgross35/rust that referenced this pull request
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
