-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add core::ptr::assume_moved #3700
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
base: master
Are you sure you want to change the base?
Conversation
a87b962
to
af48452
Compare
Is there a reason we can not just say changing the upper bits has no impact on a pointer if an appropriate tagging scheme is available, without need for additional methods? |
Yes, the reason is that even if the hardware understands a particular tagging scheme, the memory model in Rust and LLVM does not. Setting a tag on a pointer, even though it has no impact on the hardware side, makes the memory model think the pointer has now been offset outside of its original allocation and thus any access to it is Undefined Behaviour. To be able to do this we need a helper method that simulates a "realloc" from the untagged address to the tagged address to make the memory model happy.
|
Cc @rust-lang/opsem |
these should probably be associated functions, not methods. also, this seems to ignore another type of pointer tagging, often used by interpreters, where the bottom bits (otherwise always zero because of alignment) are used to tag the type of the object. |
This question should indeed be answered in the RFC text, not just in the discussion thread. (This RFC could have benefited from a pre-RFC phase, posting it on the forum to get some feedback to ensure that it has all the expected details.) |
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.
Thanks for posting the RFC! However, I think it wasn't quite ready yet. Here's some first feedback. This RFC is still lacking most of the relevant details, so I didn't proceed beyond the reference-level section.
text/3700-ptr-tag-helpers.md
Outdated
# Summary | ||
[summary]: #summary | ||
|
||
Add helper methods on primitive pointer types to facilitate getting and setting the tag of a pointer. |
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 term "tag of a pointer" means a lot of different things, and often it means something different from how you are using the term here. So the RFC should clarify the terminology it uses. This is, AFAIK, not meant to be a general pointer tagging mechanism. No RFC is even needed for that. It is specific for working with hardware that ignores certain bits of a pointer.
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 thinking is that hardware that ignores certain bits of a pointer is only relevant if you're setting those bits to some values, i.e. in the context of pointer tagging. Thus, the way to add support for such hardware is through a mechanism for pointer tagging. Does that reasoning make sense?
This is still a bit of an open question, there are several directions we could go with this. The tricky part is that different architectures have support for something similar, but the details (which exact bits are ignored) may vary.
On that account I'm not sure whether it'd be better to try and make a general high-bit pointer tagging mechanism that could be used across architectures, or whether it'd be better to just upstream e.g. aarch64-specific functions into core_arch and maybe then think about a generic one that just calls into those depending on the platform.
I don't really have very strong opinions here.
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'm just pointing out the RFC is written in a confusing way. I would suggest to make it very targeted specifically for "supporting hardware that ignores some bits in a pointer", rather than general pointer tagging. This should become clear already in the summary and the first paragraph of the motivation.
text/3700-ptr-tag-helpers.md
Outdated
the lower 48 bits, leaving higher bits unused. The remaining bits are for the most part used to | ||
distinguish userspace pointers (0x00) from kernelspace pointers (0xff). | ||
Certain architectures provide extensions, such as TBI on AArch64, that allow programs to make use of | ||
those unused bits to insert custom metadata into the pointer. |
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.
To be clear, userspace can already do that without hardware support, trivially. By masking out those bots for each load. The hardware feature just makes this slightly more efficient.
I find the introduction to be a bit confusing due to this.
text/3700-ptr-tag-helpers.md
Outdated
Currently, Rust does not acknowledge TBI and related architecture extensions that enable the use of | ||
tagged pointers. This could potentially cause issues in cases such as working with TBI-enabled C/C++ | ||
components over FFI, or when writing a tagging memory allocator. |
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.
Currently, Rust does not acknowledge TBI and related architecture extensions that enable the use of | |
tagged pointers. This could potentially cause issues in cases such as working with TBI-enabled C/C++ | |
components over FFI, or when writing a tagging memory allocator. | |
Currently, Rust does not support directly using TBI and related architecture extensions that simplify the use of | |
tagged pointers. This could potentially cause issues in cases such as working with TBI-enabled C/C++ | |
components over FFI, or when writing a tagging memory allocator. |
text/3700-ptr-tag-helpers.md
Outdated
Certain architectures provide extensions, such as TBI on AArch64, that allow programs to make use of | ||
those unused bits to insert custom metadata into the pointer. | ||
|
||
Currently, Rust does not acknowledge TBI and related architecture extensions that enable the use of |
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 RFC should explain what the problem with the use of these extensions in Rust is.
Also, it is very odd to see interop with C/C++ as the motivation here, given that those languages do not support TBI either (which is another relevant fact the RFC should mention).
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.
Well they don't support it "fully" on a language level, as in you can't just tag arbitrary pointers with no issues, but they sort of do in that there are currently C/C++ components running in production that operate on TBI-enabled pointers.
I want to make it possible to, say, write a custom allocator which internally just calls malloc, puts some tag in the pointer it got from malloc and then returns that pointer to the user.
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 don't doubt there's C/C++ code in production that causes UB and works anyway, but since we are going for a UB-free approach here as we always do, it is not a fair comparison to claim that C/C++ already supports this in a way Rust wouldn't.
text/3700-ptr-tag-helpers.md
Outdated
``` | ||
assert!(ptr.tag() == 0); | ||
let tagged_ptr = unsafe { ptr.with_tag(63) }; | ||
assert!(tagged_ptr.tag() == 63); |
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 guide-level explanation should give a guide for how to think about these functions and what they do.
This is where you have to explain that tag
is basically realloc
. Currently, realloc
suddenly appears in "drawbacks", which nobody reading the RFC will understand.
Remember that an RFC is supposed to be a self-contained document such that everyone who is reasonably well-versed in Rust but knows nothing about the particular problem domain can understand what problem the RFC is trying to solve, and how it is trying to solve it. Your RFC is missing a lot of background and explanation to make it satisfy this requirement.
text/3700-ptr-tag-helpers.md
Outdated
[reference-level-explanation]: #reference-level-explanation | ||
|
||
Within Rust's memory model, modifying the high bits offsets the pointer outside of the bounds of | ||
its original allocation, making any use of it Undefined Behaviour. |
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.
its original allocation, making any use of it Undefined Behaviour. | |
its original allocation, making any load/store with that pointer Undefined Behaviour. |
"any use" is not correct, e.g. wrapping_offset
or ==
on such a pointer are completely fine.
text/3700-ptr-tag-helpers.md
Outdated
caller. | ||
|
||
# Reference-level explanation | ||
[reference-level-explanation]: #reference-level-explanation |
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 section should contain the full signature of the newly proposed methods, and their doc comments (see the existing pointer methods for how our doc comments look like), so that we have an exact description of what the proposal even is. You can't just propose two function names and leave the details for later, when the entire reason that an RFC is even needed is that those details are far from simple.
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.
Yes for sure, the reason they're not there yet is that I wanted to ask for outside opinions before settling in on what those signatures should be and what the exact methods should actually look like. My bad that it didn't come across as intended.
The concrete part of the proposal is that we should have a function/method to set the high-bit tag of a pointer & one to retrieve it, the other details such as the name, where it should live or what it should entail are still in flux.
text/3700-ptr-tag-helpers.md
Outdated
[drawbacks]: #drawbacks | ||
|
||
Because the memory model we currently have is not fully compatible with memory tagging and | ||
tagged pointers, setting the high bits of a pointer must be done with great care in order to |
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.
That's not correct. Pointer tagging works perfectly fine and is a commonly used technique in Rust. The memory model is just not compatible with loading from a tagged pointer without first removing the tag.
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.
That's why I wrote "not fully compatible" - you can make it work, just not as smoothly as one would like given what the hardware can support.
Besides, if you remove the tag, would that not make the result a different pointer? They won't compare equal after all.
Either way, I can clarify for sure if it's not self evident.
Why can't this be done by the backend? ie. I write my code as |
Is simulating a Consider the following code: void *original = /*...*/;
void *copy = original;
void *tagged = realloc(original, ...); Now, according to the semantics of However, if my understanding of TBI is correct, this is not what happens here. Specifically, Am I misunderstanding TBI or |
you have UB if you try to do any accesses through |
@matthieu-m this model definitely makes some code UB that would be correct when using TBI in an assembly program. However, we have to impose some restrictions to make TBI compatible with higher-level language models such as Rust (and the same goes for C and C++). |
The discrepancy caused by LLVM (and Rust) not understanding the concept of TBI is fairly unfortunate. I think it should be noted in Future Possibilities that the choice of using a That is, while overly restrictive today, the drawback of the selected model is not painting us into a corner as far as I can see. |
Why is this better than explicitly masking off the bits and then having that mask be optimized away? |
Well, sometimes they get ignored, and sometimes all bits matter. This seems highly non-trivial, but I am not an expert on the relevant LLVM passes.
That also sounds like an option, if LLVM supports it. |
It seems like LLVM knows about it, but doesn't currently have a pass that optimizes for it: Seems like it would make more sense to add this functionality to LLVM rather than Rust though. |
Indeed, I should have at least marked it as draft from the get-go, or started with the forum as you suggest. This was intended as a conversation starter, it's by no means a ready proposal. I'm fully expecting to re-write this with more information, just want to get some outside opinions and fresh eyes on the direction first. |
That does sound like something that could be a useful LLVM pass, especially for compatibility with different platforms.
The snippet above will currently compile & work "fine" on a TBI system, except that Miri will rightly complain that the code has UB. The end goal of this proposal is to create an interface for top-byte tagging that does not break the memory model. This is separate from making those always safe to dereference, for which the LLVM pass would be helpful. |
It's not different. Methods already exist to do what you are trying to do: fn mask_addr(addr: usize) -> usize {
addr & 0xFFFFFFFFFFFFFF
}
let tag = 60;
let tagged_ptr = (&value as *const _).map_addr(|addr| addr | (tag << 56));
let val = unsafe { *tagged_ptr.map_addr(mask_addr) }; This is completely sound under MIRI and doesn't require any extensions to the Rust abstract machine. The only bit that's missing is a compiler optimization that erases the |
If I'm understanding what you're suggesting correctly, under this model the users would need to write out |
@RalfJung Should I then re-write this based on the already received comments and then post on Rust Internals? |
Fair enough - the proposed API seems a bit too high level for this allocator use-case though? Wouldn't the primitive operation be something like "realloc" but where you specify the target address? And there doesn't need to be an explicit |
It certainly could be if that's the community consensus, I don't have very strong views on what the exact API should look like - my intention when posting this was to get opinions on that exact question. Next time around I'll go through Internals first, I suppose I took the request for comments term a bit too literally for how it's used here :)
True as well, the intent there is just for convenience. Because different architectures can use different bits for the tagging it'd make sense to have a corresponding |
I also think a lower-level API that focuses on the |
The realloc approach would also support cases where virtual memory mappings are used for a similar purpose on platforms without hardware support for pointer tagging (ie. where you map the same physical memory to two or more virtual address ranges). |
I don't think Rust will have standard APIs for manipulating page tables? ;) I was going to say, I don't think this is ready yet for a portable API. It makes little sense to try and sketch a portable API that has exactly one target implementation. The RFC should focus in providing APIs for platform-specific capabilities, e.g. in |
that doesn't matter if you can still use This is kinda similar to how Rust doesn't have a |
|
i meant that you'd |
af48452
to
c619803
Compare
Updated the RFC based on the discussion on internals: |
#[unstable(feature = "ptr_assume_moved", issue = "none")] | ||
#[cfg_attr(not(bootstrap), rustc_simulate_allocator)] | ||
#[allow(fuzzy_provenance_casts)] | ||
pub unsafe fn assume_moved<T>(original: *mut T, new_address: usize) -> *mut T { |
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.
Bikeshed: I find "assume" to be not a great choice here, since usually "assume(X)" means "either X is true or we have UB". But here it's actually fine for the allocation to not have moved at all.
It's more like, we are telling the AM that a move happened. I found "simulate" a better term than "assume". We could also use some other term of art like "axiomatic_move" or "abstract_move".
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 think this is mostly subjective, arguments can be made for several different names. As I said in the internals thread, if someone from the libs/opsem team blesses a particular name I'm happy to just change it to that, whatever it may be. I don't have a particularly strong preference of my own.
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.
It's also easy to rename later so the name does not have to be final in the RFC. (That should be mentioned under "open questions".)
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.
Added that section back as requested :)
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 usually "assume(X)" means "either X is true or we have UB". But here it's actually fine for the allocation to not have moved at all.
The assume_moved
name is growing on me, since I'm struggling to come up with a better one. The way I read it in this context is that the compiler gets to assume that the programmer is treating the "move" as having happened, and in particular, that 1) the memory was copied and 2) the programmer is not going to dereference that original pointer after the call.
So in your model, that's what I fill in for "X" here. Either the programmer set things up as though for a move and, after this call, will behave as though a move happened, or we have UB.
That seems to align with the current safety comment:
SAFETY: Users must ensure that
new_address
actually contains the same memory as the original... after using this function, users must ensure that the underlying memory is only ever accessed through the newly created pointer. Any accesses through the original pointer (or any pointers derived from it) would be undefined behavior.
is straightforward to write a 'correct' pointer tagging implementation by simply doing a `inttoptr` | ||
cast inside the memory allocator implementation, be it a custom C `malloc` or using a custom C++ | ||
`Allocator`. The goal of this effort is to create a Rust API for implementing this type of | ||
functionality that is guaranteed to be free of Undefined Behaviour. |
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.
FWIW doing this in the global_allocator in Rust is "probably also fine", same as in C/C++. Those are not inlined and so the compiler won't see the problematic pointer manipulations. The allocator is already special and it could be possible to fold this magic into that existing magic. (It'd still be nicer to do something proper. I just don't want there to be the impression that Rust somehow supports this less than C/C++ do.)
The main case where new language support is required is when doing this inside regular code.
c619803
to
b3995b9
Compare
Should I ping someone or make a thread on Zulip or something of the sort? There's been no movement for a good while, just wanna make sure it doesn't get lost. |
unsafe { | ||
asm!( | ||
"/* simulate a move from {original} to {ptr} */", | ||
original = in(reg) original, | ||
ptr = inout(reg) ptr | ||
); | ||
} |
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 equivalent to just calling black_box
?
Give that black_box
is documented as just a hint, this should be marked as a hack that we use for codegen backends which don't have a proper way to represent this operation.
We probably want to make this an intrinsic, and then each backend can decide to implement this either like black_box or in a more direct way. The RFC shouldn't specify the exact implementation, but instead describe the concerns backends have to be aware of here.
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'm not sure whether it's 1:1 equivalent to black_box, this was originally suggested by @Amanieu as a solution to the problem. I'll update the RFC with a note about this being just an example implementation and the concerns as requested.
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 expect it to not be equivalent to black_box
(since that's documented to be just a hint), but to be equivalent to the inline asm that black_box
is currently implemented with since rustc and llvm theoretically have no way of knowing you aren't calling realloc
from inside the inline asm.
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.
Yes that's what I meant, I should have been more precise -- a backend can decide that its black_box impl is sufficient for this purpose, but a user cannot assume them to be equivalent.
From an opsem perspective this is coherent, as far as I can tell. @rust-lang/opsem please chime in if you have any comments. Nominating for @rust-lang/lang to get some idea of their thoughts on this language extension. |
Co-authored-by: Ralf Jung <post@ralfj.de>
@rustbot labels -I-lang-nominated +I-lang-radar We discussed this in the lang call today with 2/5 present (and with @Amanieu). We generally felt positively about this. We understood and agreed with the use case. On the name, while there was some initial skepticism of "move" being in there, it ended up growing on everyone after considering how this is tied in with making a guarantee about (the lack of) aliasing. Separately, we agreed with RalfJ's reservations about "assume", given the signature. |
Thank you for the update! Is there anything I should do on that account, or are we giving it more time before taking any specific steps? |
We didn't have any specific asks. We'll probably just need to have a closer look, and maybe bikeshed the name a bit, before proposing FCP. |
challenge for Rust, as the memory model still considers those high bits to be part of the address. | ||
Thus, from the memory model's perspective, changing those bits puts the pointer outside of its | ||
original allocation, despite it not being the case as far as the hardware & OS are concerned. This | ||
makes loads and stores using the pointer Undefined Behaviour, despite the fact that if such 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.
makes loads and stores using the pointer Undefined Behaviour, despite the fact that if such loads | |
makes loads and stores using the pointer undefined behavior, despite the fact that if such loads |
Please change this throughout. We don't capitalize "undefined behavior" just as we don't capitalize "race condition". (And we generally go with the American English spelling.)
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 lots of capitalized "Undefined Behavior" in the libstd doc e.g. https://doc.rust-lang.org/1.86.0/std/primitive.pointer.html#safety-9 maybe those should also be lowercased
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.
Yes, we've been cleaning this up incrementally. Feel free to put in a PR there (and ping me on it).
To ensure that the function actually has the desired behaviour, we need to make sure that it is | ||
treated similarly to real allocator functions in the codegen stage. That is to say, the function | ||
return value must be annotated with `noalias` in LLVM, as explained earlier in this section. One way | ||
to do so would be through a rustc built-in attribute similar to e.g. `rustc_allocator` - | ||
`rustc_simulate_allocator`. This attribute will be passed down to the codegen stage so that the | ||
codegen can appropriately annotate the function. |
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.
So how does this interact with allocation-eliding optimizations?
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 expect it doesn't have any allocation-eliding optimizations in LLVM, since those require more than just noalias
on the return type, e.g. malloc
has the additional function attributes:
allockind("alloc,uninitialized") allocsize(0) "alloc-family"="malloc"
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 sentence in the RFC makes little sense: a noalias
return attribute only adds more UB. So this attribute can never be required to ensure desired behavior, it can only be required to ensure more optimizations.
What exactly should the helper be called? The main candidates so far have been `simulate_realloc`, | ||
`simulate_move` and `assume_moved`, alongside some other suggestions, but preferences seem to be | ||
rather subjective and no clearly most suitable name has emerged as of yet. It can be renamed to | ||
whichever name the relevant Rust team finds most appropriate. |
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.
Let's please move discussion of the name to the rationale and alternatives section. I'd prefer we make a presumptive decision on this at the RFC step and not leave it as an unanswered question.
What is the best way to make this compatible with Strict Provenance? We want to be able to create a | ||
pointer with an arbitrary address, detached from any existing pointers and with a brand-new | ||
provenance. From the LLVM side this can be handled through generating `inttoptr` which does not have | ||
the same aliasing restrictions as `getelementptr` alongside annotating the function return value as | ||
`noalias` which can be done with the aforementioned new built-in attribute. Is this enough for it to | ||
fit within the Strict Provenance framework? If not, how can we make it fit? |
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, what are your thoughts on this? Do we need to leave this as an open question, or do we have any answers here? It would seem a substantial open question to leave.
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 can't follow the arguments in the text as all -- what it proposes for LLVM makes little sense to me. I'd like to hear from @nikic what the best way would be to model this for LLVM; my proposal would be an inline asm block.
From the Rust side, we can just say that this is a realloc
with a chosen address (and it is UB if that would make the allocation overlap with some other allocation). I don't know which potential problems with strict provenance the RFC refers to.
This RFC adds one associated function to `core::ptr`: | ||
`pub unsafe fn assume_moved<T>(original: *mut T, new_address: usize) -> *mut T` | ||
|
||
``` |
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.
``` | |
```rust |
Please add this below as well.
I've now had the opportunity to review this thoroughly. It's well written and well motivated. Thanks both to the author for that and to @RalfJung, who asked a lot of the right questions above to sharpen this up. As discussed above, I'd like us to pare down the unanswered questions to the degree possible. Personally, I'm happy enough with the name After those points are addressed, I'm planning to propose FCP merge on this RFC. |
Add a helper for primitive pointer types to facilitate modifying the address of a pointer. This
mechanism is intended to enable the use of architecture features such as AArch64 Top-Byte Ignore
(TBI) to facilitate use-cases such as high-bit pointer tagging. An example application of this
mechanism would be writing a tagging memory allocator.
Rendered