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

Replace offset with wrapping_offset #79

Open
joshlf opened this issue Sep 18, 2017 · 3 comments
Open

Replace offset with wrapping_offset #79

joshlf opened this issue Sep 18, 2017 · 3 comments

Comments

@joshlf
Copy link
Collaborator

joshlf commented Sep 18, 2017

Currently we use offset in a number of crates. However, it is unsafe to use offset to implement an allocator because, according to the docs, "both the starting and resulting pointer must be either in bounds or one byte past the end of an allocated object."

Thus, we should switch to using wrapping_offset, which does not have this requirement.

According to the wrapping_offset docs, offset allows the compiler to make more aggressive optimizations, so we should be careful that this change does not regress performance.

@ezrosent
Copy link
Owner

ezrosent commented Sep 18, 2017

It is not clear to me given the docs for offset and wrapping_offset that our uses of it are incorrect.

Except in cases where the caller passes an invalid pointer to free (which would result in Bad Things regardless of which offset method we use), I would consider any offset calls in the code resulting in a pointer to invalid (i.e. not mapped) memory to be a bug. Using wrapping_offset would not eliminate such a bug.

You might have a different interpretation of what the docs mean when they say "in bounds or one byte past the end..." than I do. I see this as meaning that we ensure pointers refer to valid/mapped memory even if that memory is an internal allocator data-structure. Indeed, we are generally careful to allocate any memory before we store a pointer to it; I don't see why memory being allocated using MmapAlloc should be considered beyond the pale when it implements Rust's (soon to be) standard allocation facility.

My intuition for this stems from the fact that the C standard has a similar provision for pointer arithmetic, and yet mallocs have historically (to my knowledge) been implemented in C using this facility.

Furthermore, the wrapping_offset docs do recommend offset as a superior default option.

@joshlf
Copy link
Collaborator Author

joshlf commented Sep 18, 2017

This was inspired by this Rust issue. @kennytm may have thoughts on this.

@joshlf
Copy link
Collaborator Author

joshlf commented Sep 18, 2017

To be clear, I think that in practice you're right that this will work, although I'm wary about relying on behavior that isn't guaranteed by the documentation.

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

No branches or pull requests

2 participants