Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Deprecate Read::initializer in favor of ptr::freeze #58363
Deprecate Read::initializer in favor of ptr::freeze #58363
Changes from 7 commits
5e0fb23
c21e79e
e4d316e
73133ee
1671e90
0529921
695feb1
9d4f07e
055bcb6
78e2233
ec682d1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #58468, I use terminology like "uninitialized memory does not have a fixed value/content", to try and distinguish it from memory that contains unknown but fixed data. Do you think it would make sense to also use such terminology here, to explain why interactions can cause UB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is that a precondition to
freeze
?freeze
does not assert validity of anything.freeze
on a*mut bool
is not UB. Just dereferencing the pointer later is.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that what you meant in this comment: #58363 (comment). If the validity requirements are less strict I can move this to be a more general warning about use of the function rather than a hard UB constraint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The phrasing here seems congruent with the behavior of
freeze::<*mut bool>(...)
tho. The value is the pointer (*mut bool
), not the pointee (bool
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validity requirements come in once you actually operate on or a make a copy of a value at some type. So the following is UB:
But it's not the freeze that is UB, it's the
into_initialized
!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RalfJung Oh... you're talking about
*mut bool
as the type of the first argument, notT == *mut bool
... i.e.ptr::freeze<bool>(x.as_mut_ptr(): *mut bool)
rather thanptr::freeze<*mut bool>(??: *mut *mut bool)
. As you have phrased it withx.into_initialized()
being UB, this seems more of a post-condition?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this formulation is used consistently throughout this file, so I'd prefer that if it gets changed that happens in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah; that's a good idea; I'll see if I can remember to make such a PR... :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a FIXME somewhere about porting this to
MaybeUninit
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This is still open, from what I can see)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the right interface? It's currently a bit weird in that we don't actually use the count. It could alternatively just take the pointer, and say that it freezes all memory reachable through it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, should this be unsafe in the first place? Since it's not actually modifying any of the pointed-to data, does it matter if it's valid or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's already "basically stable", I wonder if this should take
&mut [T]
and move tostd::mem
?I'd naively think that it could be safe and probably should be, but I'm not an expert!
We also briefly discussed maybe only taking
u8
for now? I'm not sure how useful this would be beyondu8
and other POD typesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should take raw pointers so people don't have to create references to uninitialized data.
count
being unused just comes from the fact that LLVM does not support "real" freeze, but I think this is a much better interface than "reachable from".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we change this to
T: ?Sized
so that you can pass a slice in? Then thecount
parameter would no longer be necessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK there's currently no way to form a
*mut [T]
without going through a reference first.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could optimize this when
count
is a constant to avoid the memory clobber by castingdst
to a[u8; count]
and usingasm!("" : "=*m" (dst) : "0" (dst))
.A memory clobber can be expensive since it forces the compiler to reload the values of all values that are potentially globally visible.
This optimization requires
count
to be properly const-propagated through inlining. Can we do this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are inlining and (limited) constant propagation passes for MIR but the former is not enabled by default and it's not clear when it will be.
I can't find any indication that your proposal will actually result in improved codegen (see this test case). Pointee types in LLVM IR are meaningless and slated for removal (if only someone would push that refactoring over the finish line), so I see no reason why the LLVM IR pointee type should have bearing on how many bytes the inline asm can write through the pointer.
Now, if there was a way to indicate that the inline asm guarantees it only accesses a certain range of bytes around the address that's passed in, that would help, but AFAIK there's no such thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I tried this out in C: https://godbolt.org/z/Fbz1o5
It seems that GCC interprets "m" constraints very precisely, while it seems that LLVM just treats all memory constraints (even read-only memory inputs) as a general memory clobber.
I suppose that in this case we can just stick to the existing formulation since there is no advantage in optimizing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it seems like this is just going to be inherently a bit imprecise until LLVM lands freeze support on its end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot review this part, as I have no knowledge of these assembly annotation.
@Amanieu could you confirm that this tells the compiler that the assembly block may read and/or write all memory reachable from
dst
, such that it can neither optimize away preceding writes nor following loads?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@comex put it succinctly here: rust-lang/rfcs#2360 (comment)
memory
makes LLVM treats this asreadnone = readonly = false
, which means that this can read and write to all memory that's not private. This includes all globals, heap memory, etc. and AFAICT definitely includes all memory reachable fromdst
.