core: Add core::ffi::c_intptr_t and core::ffi::c_uintptr_t

[lygstate]

View all comments

In summary, I want to give the following conclusion:

1. usize, isize should be the same with C23's size_t ssize_t as @tgross35 said at comment.
   And from https://internals.rust-lang.org/t/pre-rfc-usize-semantics/19444/156

2. (c_intptr_t, c_uintptr_t) be defined as one alias of (i8, u8), (i16, u16), (i32, u32), (i64, u64), (i128, u128) in core::ffi is enough.

3. c_ptrdiff_t is very special, can be either ssize_t or intptr_t

4. Add function expose_addr to pointer Trait for retrieve the raw integer value for the tagged-pointer.

impl *const T {
    fn addr(self) -> usize;
    fn expose_addr (self) -> c_uintptr_t;
    // ...
}

fn cast_between_pointer_uintptr() {
    let my_val = 42;
    let ptr = &my_val as *const i32;

    // Convert pointer to isize
    let tagged_ptr_integer = ptr as c_uintptr_t;
    let ptr2 = tagged_ptr_integer as *const i32;
}

maybe the as operator is enough.
And avoid cast pointer from/to usize/isize by stablize Tracking Issue for strict_provenance_lints

5. Add a function to convert c_uintptr_t, c_intptr_t back to pointer. Maybe the as operator is enough.

6. ptraddr_t is just a alias of usize, do not know if this need to be in core::ffi, not part of C standard yet, but Rust is a language for safety, add it into core::ffi seems make sense.

7. The usage of usize/isize is guard by strict_provenance

Tracking issue: #88345

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @scottmcm, libs
• @scottmcm, libs expanded to 8 candidates

[tgross35]

There are some unresolved questions there that unfortunately haven't made much progress. I'm not sure it's worth adding more types without having a better story there.

[lygstate]

There are some unresolved questions there that unfortunately haven't made much progress. I'm not sure it's worth adding more types without having a better story there.

libc is looking for release 1.0 I think it's time to resolve those questions. What's the questions comes from, can you give me the link.

[tgross35]

libc is looking for release 1.0 I think it's time to resolve those questions. What's the questions comes from, can you give me the link.

As far as libc 1.0 is concerned, the only thing I'd like is for the lang team to say that size_t is always usize, if that's something we can rely on, because then we can delete the size_t alias. Anything else is not blocking, we can update aliases if/when things stabilize.

Info is scattered, see discussion around #88345 (comment) https://internals.rust-lang.org/t/pre-rfc-usize-semantics/19444/22, https://rust-lang.zulipchat.com/#narrow/channel/213817-t-lang/topic/Compat.20with.20size_t.2C.20ptrdiff_t.2C.20intptr_t.2C.20fnptr-sized.20int/with/547982505, and a few other places on Zulip.

[arichardson]

CC: @xdoardo

[tgross35]

For anybody who has opinions here about CHERI or otherwise, it would be good to hop in to the ongoing discussion at #t-lang > Compat with size_t, ptrdiff_t, intptr_t, fnptr-sized int.

[arichardson]

Thanks for this update, I like the transparent struct approach.

View changes since this review

[arichardson]

Suggested change

	let ptr = core::ptr::with_exposed_provenance(16_usize);
	let ptr = core::ptr::without_provenance(16_usize);

[arichardson]

I think it would make a lot of sense to add a usize/isize constructor as well that creates a without_provenance pointer. This would make it more ergonomic to call APIs that use something like "valid pointer" or magic integer (e.g. -1).

[RalfJung]

(c_intptr_t, c_uintptr_t) be defined as one alias of (i8, u8), (i16, u16), (i32, u32), (i64, u64), (i128, u128) in core::ffi is enough.

So is this a guarantee that it's never (isize, usize)? That seems odd, why exclude that type?

[lygstate]

(c_intptr_t, c_uintptr_t) be defined as one alias of (i8, u8), (i16, u16), (i32, u32), (i64, u64), (i128, u128) in core::ffi is enough.

So is this a guarantee that it's never (isize, usize)? That seems odd, why exclude that type?

Now I realized it, when I create this MR, I just followed the ISO-C; maybe we need introduce something like target_address_width, when target_address_width == target_pointer_width, then c_intptr_t::integer and c_uintptr_t::integer is just an alias of (isze, usize), otherwise it's one alias of (i8, u8), (i16, u16), (i32, u32), (i64, u64), (i128, u128)

[xdoardo]

maybe we need introduce something like target_address_width, when target_address_width == target_pointer_width

For what it is worth, in the cheri-rust repo we are experimenting with this mechanism too: CHERIoT-Platform/cheri-rust#138. Let me explicitly mention @seharris, who might have interesting comments on this PR.

[rust-bors]

☔ The latest upstream changes (presumably #157558) made this pull request unmergeable. Please resolve the merge conflicts.

[Mark-Simulacrum]

r? tgross35

I think there's been a bunch of discussion on Zulip and I'm not quite sure what the current state is. I'll move reviewer to you but feel free to re-roll libs or nominate for team(s) -- I suspect we want some kind of approval of the API surface area (libs-api ACP?).

[rustbot]

tgross35 is currently at their maximum review capacity.
They may take a while to respond.

[lygstate]

I am watching this, waiting for libs-api ACP, do I need create RFC for this?

[programmerjake]

if this is a library feature, afaik it doesn't need to be added here

View changes since the review

[programmerjake]

Suggested change

	use crate::mem::{self};
	use crate::mem;

View changes since the review

[programmerjake]

this definition could have an incorrect ABI on some targets since some targets require integers to be sign/zero-extended to fill a register depending on the type's signedness, but if pointers are smaller than a register there is no way for rustc to know if it should sign or zero-extend.

View changes since the review

[RalfJung]

Yeah, pointers are not ABI-compatible with any integer type.

[lygstate]

Oh, so this is a ABI break, c_intptr_t must be a integer after-all, uintptr_t is ABI in-compatiable with *const () for FFI passing? This is solely for FFI usage, not for writing code. does this will affect FFI-parameter passing? @arichardson , It's you suggested

[RalfJung]

This type should be defined as a newtype around isize on current targets. If we ever have upstream CHERI support, then on that target it should be a newtype around *const (). That's pretty much exactly what clang does as well.

[lygstate]

Oh, I see, so for default, c_intptr_t is just a alais of isize? do I need pub struct c_intptr_t(isze);? The previous one should be compatiable with libc, but we would have non-consistence API cross current targets/(CHERI like targets)

[arichardson]

I agree that isize/usize seems like the safer default. Extension behaviour on 32-on-64 ABIs such as x32 or MIPS n32 mean that using pointers as the underlying type could potentially be wrong (I'd need to double check). What I would really like is if this could remain a somewhat opaque struct with .ptr/.integer accessors so that it's clear that this should be used for FFI calls with unclear typing (could be integer or pointer) and is not just an integer type alias.

[lygstate]

I agree that isize/usize seems like the safer default. Extension behaviour on 32-on-64 ABIs such as x32 or MIPS n32 mean that using pointers as the underlying type could potentially be wrong (I'd need to double check). What I would really like is if this could remain a somewhat opaque struct with .ptr/.integer accessors so that it's clear that this should be used for FFI calls with unclear typing (could be integer or pointer) and is not just an integer type alias.

Maybe we need introuce target_address_width first in rust mainline, with that we can default to usize/isize with target_address_width ==target_pointer_width.

[programmerjake]

Maybe we need introuce target_address_width first in rust mainline, with that we can default to usize/isize with target_address_width ==target_pointer_width.

all currently existing rust targets have matching target_address_width == target_pointer_width, so we can just use:

#[repr(transparent)]
struct uintptr_t(usize);

and fix it when we add a target where that's not true.

[lygstate]

Thanks, that's the answer I want, now I can moving forward.

[seharris]

I think I'd like there to at least be a note in documentation that relying on this being an integer isn't future proof (given that, I think, this exposes the usize field to user code), just to avoid creating any new implied API guarantees that people might rely on, and that CHERI ports will then be forced to break. (I'm imagining the future fix for this would be to use #[cfg] to swap this out for struct uintptr_t(*const ()) on CHERI)

[programmerjake]

this trait should probably be sealed

View changes since the review