Add checked arithmetic operations traits by ncatelli · Pull Request #298 · rust-osdev/x86_64

ncatelli · 2021-08-19T18:49:30Z

General

This is an initial attempt to define a CheckedAdd and CheckedSub set of traits per the traits discussed in the linked issue below. I've applied these traits to the ~~VirtAddr~~ addressing types listed in the issue to check address, page and frame arithmetic.

Related Issues

#293

josephlr

Thanks for this PR. My comments are mostly doc/implementation nits.

Note that while #299 makes some of the implementation tricky. I don't think we have to wait on that bug to be fixed before submitting these changes.

ncatelli · 2021-08-20T14:46:35Z

I've made all requested changes and have mostly stuck to your requests directly, the only difference would be retention of the chain in the VirtAddr::checked_* methods due to the from_canonical change keeping the chain fairly readable.

josephlr · 2021-08-22T08:37:16Z

@ncatelli I've resolved all the comments that you've fixed (I think there are still a few more comment nits to fixed).

Can you add implementations for the other address types (PhysAddr, PhysFrame, Page)

josephlr · 2021-08-22T09:06:33Z

Also @ncatelli per #293 (comment) can you remove the usize implementations here.

ncatelli

Sorry about that. I had over looked the other changes you'd requested, these are now fixed.

I've implemented the trait for the additional types you requested for the types laid out in #293. I've left a few comments regarding preference questions on a few of the check calls. Since most of these questions apply to the implementations of both Page and PhysFrame, I've only left discussion comments on PhysFrame.

Again, thanks again for all the time, I know this PR and attached issue have been fairly high-touch.

ncatelli · 2021-08-22T19:52:15Z


 impl<S: PageSize> PhysFrame<S> {
-    /// Returns the frame that starts at the given virtual address.
+    /// Returns the frame that starts at the given physical address.


Small typo that I'd noticed and corrected. I just wanted to call it out explicitly since this was beyond the scope of the PR and just to spot-check my change

ncatelli · 2021-08-22T19:52:31Z


    const_fn! {
-        /// Returns the frame that starts at the given virtual address.
+        /// Returns the frame that starts at the given physical address.


Same as above, additional doc change.

ncatelli · 2021-08-22T20:03:35Z

+    fn checked_sub(self, rhs: PhysFrame<S>) -> Option<Self::Output> {
+        self.start_address()
+            .checked_sub(rhs.start_address())
+            .map(PhysAddr::new)


I opted for new, which can panic over try_new as the sub should prevent ever falling into panic inducing address space unless someone is doing something really bananas. I can happily change this to try_new if you would prefer since new just calls try_new under the hood. This would guarantee we entirely avoid a panic that I think won't ever even happen, but at the expense of denser code.

Nevermind. The more i thought about this I opted to change it. The guarantee of no panic for a checked_* method should probably be the default.

What is the point of converting the offset to a PhysAddr and PhysFrame? Couldn't you just write this?

self.start_address() .checked_sub(rhs.start_address()) .map(|offset| offset / S::SIZE)

Hello, I opted for the case of safely validating of the address and the alignment as there are a few cases where an unaligned frame start address could be returned. This would necessitate the caller broke the contract of from_start_address_unchecked. I'm happy to change this method if you feel strongly about it.

Because from_start_unchecked is marked as unsafe, we can always assume that the caller did not violate the safety requirements and we don't need to check for them again.

Even if the caller did violate the requirements, this would cause the function to fail as if an underflow occurred even when that's not the case, which seems a bit unexpected.

Fair, the change you've requested has been made.

I noticed that you also used this pattern in a few other places, those probably need to be changed too.

Applied the same change to Page::checked_sub to match the same. By other's i assume you mean something like https://github.com/rust-osdev/x86_64/pull/298/files#diff-d2a15219d98ad66c2aeac0ee03d682034bc619fe5905e63960599aa58b7a63cfR332-R342, in which this was done for the sake of checking valid addresses of the multiple and I think there is no reason to change that.

You don't need to check if the offset is a valid address, in fact it is wrong to do so. For addition it's perfectly fine to jump from the lower half of the address space to the upper half. In that case the offset is not necessarily a valid virtual address. Offsets just aren't the same as virtual (or physical) addresses.

I'm aware that there has been some discussion on whether we should allow jumping from the lower half to the upper half and I'm open to change in that direction. However for now we should make sure that the checked and non-checked implementations behave in the same way.

ncatelli · 2021-08-22T20:10:48Z

+    fn checked_add(self, rhs: u64) -> Option<Self::Output> {
+        let phys_addr_rhs = rhs
+            .checked_mul(S::SIZE)
+            .and_then(|addr| PhysAddr::try_new(addr).ok())?;


Let me know if this is overkill and you'd like me to opt for a lighter weight checked op by removing the PhysAddr validity check.

ncatelli · 2021-08-23T00:56:47Z

Alright, that should be the last of my changes until the next review cycle. I wanted to make sure intent was either clear or documented with examples (that hopefully make it clear).

ncatelli · 2021-08-24T20:51:51Z

One additional note, while I'm aware of the discussion in #293 about removing the usize impls for Add and Sub this PR doesn't currently contain that since it was still under discussion, and I'd assumed out of scope. I'd be happy to add that or more preferably provide a follow up PR with those changes. Additionally I'm working off the assumption that #299 and the corresponding jumping_add method/trait fell out of scope of this PR.

phil-opp · 2021-11-06T13:54:41Z

Triage: This PR has been waiting for some time. @josephlr Do you have time for a new review, or should I try to take over?

ncatelli commented Aug 19, 2021

View reviewed changes

Comment thread .gitignore Outdated

ncatelli mentioned this pull request Aug 19, 2021

Define CheckedAdd and CheckedSub traits #293

Open

ncatelli commented Aug 19, 2021

View reviewed changes

Comment thread src/addr.rs Outdated

ncatelli force-pushed the expermiment/293/add-checked-arithmetic-operations-traits branch from 137cdef to 76ca2e9 Compare August 19, 2021 20:24

ncatelli commented Aug 19, 2021

View reviewed changes

Comment thread src/lib.rs

josephlr mentioned this pull request Aug 20, 2021

VirtAddr::try_new() succeeds on non-canonical addresses #299

Closed

josephlr requested changes Aug 20, 2021

View reviewed changes

josephlr mentioned this pull request Aug 22, 2021

Add checked_add to VirtAddr and update frame_to_pointer #291

Closed

ncatelli commented Aug 22, 2021

View reviewed changes

ncatelli requested a review from josephlr September 8, 2021 22:55

phil-opp added the waiting-for-review Waiting for a review from the maintainers. label Nov 6, 2021

Nate Catelli added 13 commits November 6, 2021 20:00

Add editor specific files to gitignore

8690c4b

Add CheckedAdd and CheckedSub

2734615

doc strings

0c35984

Implement Checked ops for PageSize

64434a7

Add checked_* operations to VirtAddr

0414650

Add a canonical check

0b55355

document is_canonical

73e564c

Removed checked from frome until VirtAddr is accepted

44e6c13

add bounds tests

7c6e66c

Move ops up from paging module for generalization.

a0f6a0a

Remove editor specific rules

02666f9

Documentation fixes per review

1dfd531

is_canonical -> from_canonical

b6c3314

Nate Catelli added 15 commits November 6, 2021 20:01

Documentation changes to ops::Checked* traits.

55d7076

Removed Checked*<usize> VirtAddr implementations

40ddc5f

Fix documentation issues on Checked* methods

648dedc

Add Checked methods to physaddr

5c7ff32

Add Checked methods for PhysFrame

cd8f720

Removed CheckedAdd<Rhs=Self> out of spec impls

e03d8f3

Correct CheckedSub<Rhs=Self> implementations to match spec

0cd24f2

flipped it back to to just convert to a u64.

94124af

Complete impls of page and frame checked_* methods

d490b84

Fix virtual -> physical address doc typo.

fecd44a

rename frame rhs for clarity

23c038a

rename virt address rhs for clarity

05c54c7

Switch *Addr::new to *Addr::try_new to avoid any risk of panic

718ea47

Fix checked subtraction of self to return difference in frames/pages

a0d3193

Add docs to the new trait impls with examples

ad70df8

ncatelli force-pushed the expermiment/293/add-checked-arithmetic-operations-traits branch from 4d95a3a to ad70df8 Compare November 6, 2021 20:03

Nate Catelli added 2 commits December 3, 2021 20:23

Simplify Frame checked_sub

1888ae2

simplify checked add for page

1b91074

ncatelli closed this Aug 14, 2023

Conversation

ncatelli commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

General

Related Issues

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josephlr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ncatelli commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josephlr commented Aug 22, 2021

Uh oh!

josephlr commented Aug 22, 2021

Uh oh!

ncatelli left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncatelli Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Freax13 Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncatelli commented Aug 23, 2021

Uh oh!

ncatelli commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phil-opp commented Nov 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ncatelli commented Aug 19, 2021 •

edited

Loading

ncatelli commented Aug 20, 2021 •

edited

Loading

ncatelli left a comment •

edited

Loading

ncatelli Aug 22, 2021 •

edited

Loading

Freax13 Dec 3, 2021 •

edited

Loading

ncatelli commented Aug 24, 2021 •

edited

Loading