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

[Fix #362] Fix sign extension bug in `next_table_address` #372

Merged
merged 2 commits into from Dec 5, 2017

Conversation

2 participants
@IsaacWoods
Contributor

IsaacWoods commented Dec 4, 2017

Fix an issue where the left shift of the old table address would overwrite the sign extension, making the
address non-canonical and leading to #GPs. This calculates the correct sign extension for the new table
address.

[Fix #362] Correct sign extension bug in `next_table_address`
Fix an issue where the left shift of the old table address would overwrite the sign extension, making the
address non-canonical and leading to #GPs. This calculates the correct sign extension for the new table
address.

@phil-opp phil-opp requested review from phil-opp and removed request for phil-opp Dec 4, 2017

@phil-opp phil-opp self-assigned this Dec 4, 2017

@phil-opp

Thanks for the PR!

Could you move the sign extension logic to a separate sign_extend function to improve readability?

@IsaacWoods

This comment has been minimized.

Show comment
Hide comment
@IsaacWoods

IsaacWoods Dec 5, 2017

Contributor

I chose to call it make_address_canonical instead, as it's specialised to address canonicalisation, rather than generic sign extension. Hope that is more readable.

Also, just something I was thinking about while implementing this, I think someone at some point mentioned the idea of replacing the aliases used in VirtualAddress and PhysicalAddress with the newtype pattern. I noticed that the addresses in the table methods use usize, which imo shouldn't be possible. If at some point you supported moving to a stricter use of the address types, I think that would increase safety and give a home to methods like make_address_canonical, and I'd be happy to do some of that legwork.

Contributor

IsaacWoods commented Dec 5, 2017

I chose to call it make_address_canonical instead, as it's specialised to address canonicalisation, rather than generic sign extension. Hope that is more readable.

Also, just something I was thinking about while implementing this, I think someone at some point mentioned the idea of replacing the aliases used in VirtualAddress and PhysicalAddress with the newtype pattern. I noticed that the addresses in the table methods use usize, which imo shouldn't be possible. If at some point you supported moving to a stricter use of the address types, I think that would increase safety and give a home to methods like make_address_canonical, and I'd be happy to do some of that legwork.

@phil-opp phil-opp merged commit cf2c555 into phil-opp:master Dec 5, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@phil-opp

This comment has been minimized.

Show comment
Hide comment
@phil-opp

phil-opp Dec 5, 2017

Owner

Awesome, thanks! I like the function name.

Regarding the newtypes for VirtualAddress and PhysicalAddress: I definitely plan to do it for the upcoming second edition (#360). The plan is to move a lot of code to the x86_64 crate and define the newtypes there. The canonical address function would fit in there pretty well, too.

Changing the first edition would be a lot of work, since it touches a lot of code and we would need to adjust almost all posts. So I think it's a better idea to focus on the second edition instead.

Owner

phil-opp commented Dec 5, 2017

Awesome, thanks! I like the function name.

Regarding the newtypes for VirtualAddress and PhysicalAddress: I definitely plan to do it for the upcoming second edition (#360). The plan is to move a lot of code to the x86_64 crate and define the newtypes there. The canonical address function would fit in there pretty well, too.

Changing the first edition would be a lot of work, since it touches a lot of code and we would need to adjust almost all posts. So I think it's a better idea to focus on the second edition instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment