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

fix extracting a u64 on little-endian CPUs #784

Merged
merged 1 commit into from
Apr 23, 2019

Conversation

RalfJung
Copy link
Contributor

The cast-to-raw-ptr-fix was wrong, but unfortunately it takes some rather special circumstances to exhibit that which did not come up in the test suite. (Miri does not do any tracking of raw pointers, so the raw pointer used here to access the second u32 probably get confused with some other raw pointer created previously. Eventually we will want to do proper tracking of raw pointers as well.)

Also, this code path can now be used on all little-endian CPUs, at the cost of an unaligned read. Is that preferred?

@RalfJung
Copy link
Contributor Author

confused with some other raw pointer created previously

Ah, it's the raw pointer created by the Index implementation for subslicing. That's why going from &results[index] to &results[index..index+1] (which covers the same 1 element!) made a difference.

The intention was to create a slice that actually covers the elements that are going to be accessed through this raw pointer, and that would be index..=index+1.

@dhardy
Copy link
Member

dhardy commented Apr 23, 2019

at the cost of an unaligned read

I guess benchmarking would be needed to know for sure whether this code path is faster, and the results would probably not be the same across all AArch64 CPUs either. I doubt it would be slower than the alternative code however, so this choice looks sensible.

Looks good. Of course, we should have realised that was a 1-elt slice before!

@vks
Copy link
Collaborator

vks commented Apr 23, 2019

We probably want to backport all those memory-safety fixes to 0.6 and add them to cargo-audit.

@dhardy
Copy link
Member

dhardy commented Apr 23, 2019

Do we? My understanding is that this is technically UB (in that it doesn't follow the spec requirements) but happens to compile to working code anyway, except when using Miri (but I may be wrong).

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