Repository navigation
Conversation
|
r? @adamgemmell rustbot has assigned @adamgemmell. Use Why was this reviewer chosen?The reviewer was selected based on:
|
032f685 to
79496f8
Compare
| /// | ||
| /// # Safety | ||
| /// | ||
| /// The current platform must support the `mte` target feature. |
There was a problem hiding this comment.
I'm not sure if the target feature requirement needs to be documented as it's more a property of the language than the function implementation. The target feature attribute should already be rendered in documentation.
There was a problem hiding this comment.
Thank you for your review. I understnd your opinion, and I think that target feature doe not need to be documented. I will update this PR.
But I think that since these APIs are marked unsafe, it's better to document these APIs and give them concrete requirement.
| /// | ||
| /// * The current platform must support the `mte` target feature. | ||
| /// * `tag_address` must be 16-byte aligned. The tag will apply to the | ||
| /// entire 16-byte memory granule. |
There was a problem hiding this comment.
According to the ACLE "These intrinsics generate an unchecked access to memory" - sic. presumably as it seems like the comment only refers to this intrinsic. Does this mean tag_address needs to be dereferenceable?
There was a problem hiding this comment.
I think this API doe not dereference the tag_address, and the actual allocated memory doe not be accessed.
| gmi_(src as *const (), excluded as i64) as u64 | ||
| } | ||
|
|
||
| /// Store an allocation tag for the 16-byte granule of memory. |
There was a problem hiding this comment.
This requires allocator support, though I'm not sure how that relates to unsafe exactly.
| @@ -27,10 +27,16 @@ | |||
| /// The returned pointer contains a copy of the `src` address, but with a | |||
| /// randomly generated logical tag, excluding any specified by `mask`. | |||
There was a problem hiding this comment.
One issue with these intrinsics is that while the hardware may consider identical pointers with different tags dereferenceable to the same address when TBI is enabled, the Rust memory model does not without rust-lang/rfcs#3700. I think it would be a good idea to draw attention to the fact that at the moment Rust will consider these pointers as pointing to different allocations.
There was a problem hiding this comment.
Thank you for your patience and review. I didn't read this rfc before so I misunderstand that Rust memory model will treat the logical tag approariately.
| /// and Undefined Behavior to dereference it. | ||
| /// This intrinsic does not access memory or change allocation tags. | ||
| /// The allocation tag can be changed using [`__arm_mte_set_tag`], | ||
| /// provided that function's safety requirements are satisfied. |
There was a problem hiding this comment.
Pointer arithmetic can still be unsafe as overflow is not defined for them, see https://doc.rust-lang.org/core/primitive.pointer.html#method.add
Granted if you're just changing tags I'm struggling to come up with an example where this can make sound instances of pointer arithmetic unsound.
There was a problem hiding this comment.
In my intuition I think that the internal irg_ directly return a pointer. So should it enforce the pointer overflow rule?
Also, the size of pointer is the same as usize (if the type is Sized). I think the returned pointer probably doe not happen overflow, because it just return a pointer whose size is the same as usize.
| #[inline] | ||
| #[target_feature(enable = "mte")] | ||
| #[unstable(feature = "stdarch_aarch64_mte", issue = "129010")] | ||
| pub unsafe fn __arm_mte_set_tag<T>(tag_address: *const T) { |
There was a problem hiding this comment.
Underlying all these intrinsics is the can of worms that the language semantics of setting tags hasn't fully been worked out yet. I don't think we have to do that as part of this PR which clearly improves things, but it's something that makes me cautious around this.
There was a problem hiding this comment.
Thanks. I didn't know the real difficulty when I look at these APIs. I will read the rfc3700 and try to improve this PR.
| /// # Safety | ||
| /// | ||
| /// * The current platform must support the `mte` target feature. | ||
| /// * The address part of `address` must identify memory whose allocation |
There was a problem hiding this comment.
What are the specific constraints on the address that must be upheld here? Can any address be used, or must the memory previously have a tag set, or is this unique to the implementation of the allocator?
There was a problem hiding this comment.
To be honest I don't know how internal ldg_ works. I don't know whether ldg_ works the same when address points at a real allocated memory or a memory that has not been allocated (in this case the allocation tag is 0). If ldg_ works the same, I think the requirement of address is that the address part of address must point at a real memory.
| #[inline] | ||
| #[target_feature(enable = "mte")] | ||
| #[unstable(feature = "stdarch_aarch64_mte", issue = "129010")] | ||
| pub unsafe fn __arm_mte_ptrdiff<T, U>(a: *const T, b: *const U) -> i64 { |
There was a problem hiding this comment.
There are lots of constraints on https://doc.rust-lang.org/std/primitive.pointer.html#method.offset_from - do any of them apply here too?
There was a problem hiding this comment.
I think the requirement of offset_from do not apply here, because subp_ is not a operation defined in Rust. It is a llvm-intrinsic.
This PR adds safety docs for pub unsafe fns in
core_arch/src/aarch64/mte.rs. There are six public unsafe functions related to aarch64 MTE (memory tagging extension). I refer to some offical guides when adding safety docs for these APIs, for example:_irg,_addg,_gmi,_subpdon't access memory,_ldgperforms read access and_stgperforms write access.For
__arm_mte_get_tag, I don't be sure that whether it needs to add the requirement that thetag_addressis valid for write. Assumingtag_addressis Null or is a pointer that is out-of-bound (For example, a pointer that is the end of a vec). But I don't really know what will happen if passing these pointers.