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

Make align_up and align_down const #270

Merged
merged 4 commits into from
Jul 16, 2021
Merged

Make align_up and align_down const #270

merged 4 commits into from
Jul 16, 2021

Conversation

josephlr
Copy link
Contributor

We can't make the {VirtAddr, PhysAddr}::{align_up, align_down, is_aligned} methods const as they have a U: Into<u64> constraint.

We should consider removing this constraint for the next breaking change.

@josephlr josephlr requested a review from phil-opp June 22, 2021 11:45
@toothbrush7777777
Copy link
Contributor

toothbrush7777777 commented Jun 26, 2021

Shouldn't these functions take (and return) usize rather than u64?

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall. Nice trick with the [(); 1] array!

One thing that is a bit unfortunate is that users on stable will get a meaningless "index out of bounds" error if they supply a non-aligned aligment. I don't think that there is any way to avoid this right now, but we should at least document this error in the method docs, e.g. as:

Panics if the alignment is not a power of two. Without the const_fn feature, the panic message will be an "index out of bounds" error in that case.

Additionally, we could add line comments before the [(); 1][!align.is_power_of_two() as usize] lines that say something like "if a panic occurs at this line it means that align was not a power of two". This way, users that look up the panic origin in the source directly see the reason for it.

@phil-opp
Copy link
Member

@toothbrush7777777

Shouldn't these functions take (and return) usize rather than u64?

They could, but there are other places in this crate where u64 makes more sense than usize. For example, the VirtAddr type requires that wrapped addresses are canonical, which is a property that is specific to 64-bit memory addresses:

x86_64/src/addr.rs

Lines 68 to 74 in e5c202c

pub fn try_new(addr: u64) -> Result<VirtAddr, VirtAddrNotValid> {
match addr.get_bits(47..64) {
0 | 0x1ffff => Ok(VirtAddr(addr)), // address is canonical
1 => Ok(VirtAddr::new_truncate(addr)), // address needs sign extension
other => Err(VirtAddrNotValid(other)),
}
}

Since conversions between u64 and usize are not really convenient (as casts risk a silent truncation; try_into can fail), we decided to use u64 as the general address type in this crate.

We can't make VirtAddr and PhysAddr methods const as they wrap an into
impl.
@josephlr
Copy link
Contributor Author

One thing that is a bit unfortunate is that users on stable will get a meaningless "index out of bounds" error if they supply a non-aligned aligment. I don't think that there is any way to avoid this right now, but we should at least document this error in the method docs, e.g. as:

Panics if the alignment is not a power of two. Without the const_fn feature, the panic message will be an "index out of bounds" error in that case.

Docs updated (along with the GDT push method).

Additionally, we could add line comments before the [(); 1][!align.is_power_of_two() as usize] lines that say something like "if a panic occurs at this line it means that align was not a power of two". This way, users that look up the panic origin in the source directly see the reason for it.

Instead of this I just added a cosnt_assert! helper macro, which does the right thing depending on if the feature is set or not. This makes the code cleaner and self-documenting.

@josephlr josephlr merged commit 7ce4d16 into master Jul 16, 2021
@josephlr josephlr deleted the align branch July 16, 2021 09:07
@phil-opp
Copy link
Member

Good solution, thanks a lot!

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

Successfully merging this pull request may close these issues.

3 participants