rust-osdev / x86_64

Library to program x86_64 hardware.
https://docs.rs/x86_64
Apache License 2.0
754 stars 130 forks source link

0.15 Tracking issue #262

Open josephlr opened 3 years ago

josephlr commented 3 years ago

This issue tracks any breaking changes we want to make for 0.15.

NOTE: All breaking changes should be made against the next branch, this branch should then be merged in right before releasing 0.15. Also, remember to update Changelog.md.

josephlr commented 2 years ago

Note that const_fn_fn_ptr_basics and const_fn_trait_bound were stabilized in 1.61

If we support only 1.61 and later, we could then remove most of our const_fn! from this crate.

@phil-opp @Freax13 would we want increase our minimum rust version to be 1.61 for v0.15.0? Would we feel comfortable increasing the minimum rust version in a patch release? Do we have a policy on this?

phil-opp commented 2 years ago

Ah, that's great! Increasing the minimum Rust version for v0.15 sounds fine to me in general, as it is a semver-incompatible release with breaking changes. However, it's still 8 (?) weeks until Rust 1.61 becomes stable, so requiring it would mean that we have to postpone this release until May if we want it to keep supporting stable Rust. So I think it would be better to do this as part of a future v0.16 release. Until then, we could use the rustversion macro to provide the new const functionality when a new enough compiler is used.

josephlr commented 2 years ago

Oh that crate looks amazing, it would let us get rid of our const_fn! macro entirely!!

If we don't need Rust 1.61 for v0.15, then I think we should have the MSRV be 1.56 so that we can use the rust-version field in our crate.

phil-opp commented 2 years ago

If we don't need Rust 1.61 for v0.15, then I think we should have the MSRV be 1.56 so that we can use the rust-version field in our crate.

Sure, sounds good to me!

josephlr commented 2 years ago

Oh wait we use inline assembly, so the min stable version is already 1.59 if the "instructions" feature is used. We should probably just make that the unconditionally minimum supported version for 0.15

josephlr commented 2 years ago

One thing I was considering for 0.15 were changes to VirtAddr's underlying representation on 64-bit platforms. If we changed VirtAddr to look like:

#[repr(transparent)]
pub struct VirtAddr(*const ());

on 64-bit platforms, we could then have the following const fn methods:

impl VirtAddr {
    // Safety: raw pointer has to be canonical
    #[cfg(target_pointer_width = "64")]
    pub const unsafe fn from_ptr_unsafe<T>(ptr: *const T) -> Self {
        Self(ptr as *const ())
    }

    #[cfg(target_arch = "x86_64")]
    pub const fn from_ref<T>(r: &T) -> Self {
        // Safety: references point to valid data, so are canonical.
        unsafe { Self::from_ptr_unsafe(r) }
    }
}

This would make it possible to construct something like a TaskStateSegment, DescriptorTablePointer, or some other structure containing a VirtAddr at compile time.

The downside of this (other than implementation complexity) is that the following methods would have to be made non-const:

I'm not sure how bad this downside would be.

phil-opp commented 2 years ago

We probably need to think about pointer provenance at some point. There is currently an open proposal to make the usize to pointer conversion more strict. See the Rust's Unsafe Pointer Types Need An Overhaul post for some background information.

I'm not sure what the correct approach for our VirtAddr type would be. On one hand, we treat it like a pointer, e.g. in the VirtAddr::as_ptr method. On the other hand, the type should be allowed to cross address space bounds (e.g. kernels that keep track of the stack pointers of preempted threads), which might (?) be invalid under strict provenance rules. So maybe we need to split this type up at some point, e.g. by removing all pointer-related methods form VirtAddr and using pointer types in more places.

Given this issue, I don't think that it's a good idea to switch VirtAddr between pointer-based and u64-based depending on the platform. It would lead to more confusion and less clear semantics.

Freax13 commented 2 years ago

Given this issue, I don't think that it's a good idea to switch VirtAddr between pointer-based and u64-based depending on the platform. It would lead to more confusion and less clear semantics.

I agree with that, having some methods only be const on 64-bit targets seems unintuitive. That being said I'd still be interested in potential use cases.

josephlr commented 2 years ago

Those articles are very interesting, and I agree that changing things in this space is really complicated. It definitely seems like keeping VirtAddr as a u64 is the best bet for now.

So maybe we need to split this type up at some point, e.g. by removing all pointer-related methods form VirtAddr and using pointer types in more places.

This is a good idea, I'll see if I can put some examples together in a separate tracking issue.

josephlr commented 2 years ago

@phil-opp @Freax13 is there anything else we want to get in before 0.15? All the currently pending PRs are additive, so could be done after we release 0.15

Freax13 commented 2 years ago

I can't think of anything.

phil-opp commented 2 years ago

Me neither!

phil-opp commented 1 year ago

What's the state of this? Should we finally prepare the 0.15 release?

Freax13 commented 1 year ago

We could wait for #404, but this also wouldn't be a deal breaker for me, especially because we don't seem to have come to a final design on the changes just yet.

Other than that, I think we're ready.

Freax13 commented 1 year ago

I'll get started on rebasing next onto master.

Freax13 commented 1 year ago

I'll get started on rebasing next onto master.

Hm, never mind, I forgot we don't do rebases. I'll get started on merging next into master instead.

Freax13 commented 1 year ago

I'll get started on rebasing next onto master.

Hm, never mind, I forgot we don't do rebases. I'll get started on merging next into master instead.

I'm done with that. Depending on whether on not we want to wait for some other breaking prs to also be included, I can create a pr.