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.

Sign up for GitHub

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

Jump to bottom

Bytes assumes that Vec<u8>'s allocation is more than 1 byte aligned #343

Closed
sfackler opened this issue Dec 12, 2019 · 7 comments · Fixed by #346
Closed

Bytes assumes that Vec<u8>'s allocation is more than 1 byte aligned #343

sfackler opened this issue Dec 12, 2019 · 7 comments · Fixed by #346

Comments

@sfackler
Copy link
Contributor

It uses the low bit of the pointer to distinguish between KIND_VEC and KIND_ARC.

Folded over from #340
@sfackler sfackler changed the title Bytes assumes that Vec<u8> more than 1 byte aligned Bytes assumes that Vec<u8> is more than 1 byte aligned Dec 12, 2019
@sfackler sfackler changed the title Bytes assumes that Vec<u8> is more than 1 byte aligned Bytes assumes that Vec<u8>'s allocation is more than 1 byte aligned Dec 12, 2019
@seanmonstar
Copy link
Member

As mentioned in #342 (comment), we'll start by adding an assertion that the LSB isn't set, and perhaps fix if/when some allocator actually starts handing out odd pointers.

@cyphar

This comment has been minimized.

Sign in to view
@sfackler
Copy link
Contributor Author

panic is not abort.

@seanmonstar
Copy link
Member

So I'm reading up on std::alloc, and this line isn't clear to me:

Layout queries and calculations in general must be correct.

Is this saying that allocators must follow alignment requests? Such that for an alignment of say 4, the returned pointer must have the 2 LSBs set to 0? Or is the details of the address 100% opaque?

@sfackler
Copy link
Contributor Author

Yes, allocators must follow alignment requirements.

@sfackler sfackler reopened this Dec 13, 2019
@carllerche
Copy link
Member

Because u8 has an alignment of 1, there is no requirement for even pointers. In practice, allocators don't allocate blocks of 1 byte because the required bookkeeping wouldn't make it worth it, however you could imagine a bump allocator returning an odd pointer.

@seanmonstar
Copy link
Member

Then since align_of::<Shared>() == 8, we could use the LSBs in that state. I can make mirrored vtables that depending on whether the LSB is set on the Vec's pointer determines what mask to use.

seanmonstar added a commit that referenced this issue Dec 13, 2019
@seanmonstar
Fix Bytes when Vec pointer's LSB is set
0db53a6 
This separates the `SharedVtable` into 3:

- `PromotableEvenVtable`: The original `SharedVtable`, which will
  promote the `Vec` to `Shared` on the first clone, and is selected when
  the `Vec`'s pointer has the LSB unset.
- `PromotableOddVtable`: Similar to the `PromotableEvenVtable`, but
  selected when the `Vec`'s pointer has the LSB set. This vtable differs
  in the masking used when reconstructing the `Vec`.
- `SharedVtable`: This no longer checks if its current kind is `VEC` or
  `ARC`, and is only created by the "promotable" vtables.

This also adds a test using an "odd" global allocator that purposefully
bumps all pointers with alignment of 1.

Closes #343
@seanmonstar seanmonstar mentioned this issue Dec 13, 2019
Merged
@Shnatsel Shnatsel mentioned this issue Dec 13, 2019
Open
seanmonstar added a commit that referenced this issue Dec 17, 2019
@seanmonstar
Fix Bytes when Vec pointer's LSB is set
30504fd 
This separates the `SharedVtable` into 3:

- `PromotableEvenVtable`: The original `SharedVtable`, which will
  promote the `Vec` to `Shared` on the first clone, and is selected when
  the `Vec`'s pointer has the LSB unset.
- `PromotableOddVtable`: Similar to the `PromotableEvenVtable`, but
  selected when the `Vec`'s pointer has the LSB set. This vtable differs
  in the masking used when reconstructing the `Vec`.
- `SharedVtable`: This no longer checks if its current kind is `VEC` or
  `ARC`, and is only created by the "promotable" vtables.

This also adds a test using an "odd" global allocator that purposefully
bumps all pointers with alignment of 1.

Closes #343
seanmonstar added a commit that referenced this issue Dec 17, 2019
@seanmonstar
Fix Bytes when Vec pointer's LSB is set
6501193 
This separates the `SharedVtable` into 3:

- `PromotableEvenVtable`: The original `SharedVtable`, which will
  promote the `Vec` to `Shared` on the first clone, and is selected when
  the `Vec`'s pointer has the LSB unset.
- `PromotableOddVtable`: Similar to the `PromotableEvenVtable`, but
  selected when the `Vec`'s pointer has the LSB set. This vtable differs
  in the masking used when reconstructing the `Vec`.
- `SharedVtable`: This no longer checks if its current kind is `VEC` or
  `ARC`, and is only created by the "promotable" vtables.

This also adds a test using an "odd" global allocator that purposefully
bumps all pointers with alignment of 1.

Closes #343
seanmonstar added a commit that referenced this issue Dec 17, 2019
@seanmonstar
Fix Bytes when Vec pointer's LSB is set (#346)
c7cf716 
This separates the `SharedVtable` into 3:

- `PromotableEvenVtable`: The original `SharedVtable`, which will
  promote the `Vec` to `Shared` on the first clone, and is selected when
  the `Vec`'s pointer has the LSB unset.
- `PromotableOddVtable`: Similar to the `PromotableEvenVtable`, but
  selected when the `Vec`'s pointer has the LSB set. This vtable differs
  in the masking used when reconstructing the `Vec`.
- `SharedVtable`: This no longer checks if its current kind is `VEC` or
  `ARC`, and is only created by the "promotable" vtables.

This also adds a test using an "odd" global allocator that purposefully
bumps all pointers with alignment of 1.

Closes #343
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix Bytes when Vec pointer's LSB is set tokio-rs/bytes
4 participants
@carllerche @seanmonstar @sfackler @cyphar