Skip to content
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

Vector Implementation #569

Open
AdinAck opened this issue Dec 30, 2024 · 7 comments
Open

Vector Implementation #569

AdinAck opened this issue Dec 30, 2024 · 7 comments

Comments

@AdinAck
Copy link

AdinAck commented Dec 30, 2024

In the cortex-m-rt docs it recommends PACs create a Vector type like so:

pub union Vector {
    handler: unsafe extern "C" fn(),
    reserved: usize,
}

and this is exactly what PACs do. I'm curious why it is like this? The reserved variant being typed as usize when it should always be 0 and constructed like:

Vector { reserved: 0 }

is very odd to me.

Why not define Vector like this:

pub struct Vector(#[allow(unused)] *const ());

impl Vector {
    /// Create a vector with the provided function pointer.
    pub const fn handler(f: unsafe extern "C" fn()) -> Self {
        Self(f as _)
    }

    /// Create a vector that is reserved.
    pub const fn reserved() -> Self {
        Self(core::ptr::null())
    }
}

and use it like this:

pub static __INTERRUPTS: [Vector; 2] = [
    Vector::handler(Foo),
    Vector::reserved(),
];

and why is this type not provided by cortex-m-rt so PACs don't have to redefine it every time?

I imagine there is some low-level issue with representing the function pointer as a unit pointer that I don't know of.

I am currently using my proposed Vector implementation and it does work, I'm just not sure if it is correct.

Thanks!

@thomasw04
Copy link

I imagine there is some low-level issue with representing the function pointer as a unit pointer that I don't know of.

It should be fine in this case. The AAPCS (default extern "C" for arm, see https://doc.rust-lang.org/reference/items/external-blocks.html#abi) defines a Code Pointer as 4 byte value (see https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#51fundamental-data-types), which corresponds to a C function pointer (see https://github.com/ARM-software/abi-aa/blob/main/aapcs32/aapcs32.rst#812pointer-types).

As *const () in Rust is not dynamically sized, it has the same memory layout as a C pointer, which corresponds to a Data Pointer in AAPCS and is also defined as a 4-byte value (see above). So, in this case, representing a function pointer as a unit pointer should be fine. (Although as far as I know, it is not explicitly allowed, at least in the C standard)

@AdinAck
Copy link
Author

AdinAck commented Jan 6, 2025

Ah, nice digging. Makes sense, thanks!

Is there a better type to use than *const () in your opinion?

@thomasw04
Copy link

Is there a better type to use than *const () in your opinion?

I mean, what about something like this:

#[repr(C)]
pub struct Vector(usize);

impl Vector {
    /// Create a vector with the provided function pointer.
    pub fn handler(f: unsafe extern "C" fn()) -> Self {
        Self(f as usize)
    }

    /// Create a vector that is reserved.
    pub const fn reserved() -> Self {
        Self(0 as usize)
    }
}

@AdinAck
Copy link
Author

AdinAck commented Jan 10, 2025

Unfortunately handler must be const since it's used in the __INTERRUPTS static.

For some reason casting pointers as integers is not permitted in const contexts. (even though it's an extern function)

@madsmtm
Copy link

madsmtm commented Feb 5, 2025

There was recently discussion about cortex-m-rt's use of a union here, see:
https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/Is.20NPO.20guaranteed.20for.20.60Option.3C*mut.20T.3E.60

What about just using Option<unsafe extern "C" fn()> nowadays? The docs call out that transmute::<_, Option<T>>([0u8; size_of::<T>()]) where T = fn() is sound and produces Option::None (i.e. that an all bit-pattern 0 represents the None), and naturally, this must also hold in reverse (that a None produces an all bit-pattern 0).

@AdinAck
Copy link
Author

AdinAck commented Feb 8, 2025

Ah, nice. From the cortex-m-rt docs:

We recommend using a union to set the reserved spots to 0; None (Option<fn()>) may also work but it’s not guaranteed that the None variant will always be represented by the value 0.

I think their concern is that the Rust guarantee that you sent is unstable, or may change between Rust releases.

So they use a union because that has to conform to the C ABI which is not going to change. A reasonable concern I suppose, but surely with a little documentation explaining how and why Option<fn()> is ok, there would be no issues until that day comes.

@madsmtm
Copy link

madsmtm commented Feb 8, 2025

I think their concern is that the Rust guarantee that you sent is unstable, or may change between Rust releases.

It's a stable guarantee.

I suspect it was just because that section has only been added fairly recently (Rust 1.74 does not go into this much detail, and the union Vector recommendation seems to be several years old).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants