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

Consider alignment when computing into_raw #138

Merged
merged 2 commits into from
Dec 16, 2023
Merged

Conversation

notgull
Copy link
Contributor

@notgull notgull commented Dec 16, 2023

This commit fixes #137 by considering alignment when converting the raw
Arc pointer to and raw its raw representation.

This commit fixes taiki-e#137 by considering alignment when converting the raw
Arc pointer to and raw its raw representation.

Signed-off-by: John Nunley <dev@notgull.net>
Signed-off-by: John Nunley <dev@notgull.net>
Copy link
Owner

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@taiki-e taiki-e merged commit ac2e68c into taiki-e:main Dec 16, 2023
98 checks passed
@notgull notgull deleted the fix-arc branch December 16, 2023 20:13
@taiki-e
Copy link
Owner

taiki-e commented Dec 16, 2023

Published in 0.1.4 & yanked old versions.

@TimNN
Copy link

TimNN commented Dec 17, 2023

FYI: I believe the new logic is still not entirely correct, at least not for arbitrary "header" and "value" types.

In particular, it fails if size_of::<Header>() is not a power of two: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=b2ba0406e00379e7986511ab3815a133

This probably isn't a significant problem right now because Header == [usize; 2], but IMO this property should either be enforced by a test, or the logic fixed.

@taiki-e
Copy link
Owner

taiki-e commented Dec 17, 2023

@TimNN Thanks for the heads up!

We don't want to define a general-purpose offset_of (we should use memoffset crate for that use case) and it would be sufficient to adopt a way that works correctly for the specific headers we use.

That said, we would at least need to add a comment about the fact that the current implementation also relies on the actual properties of the header.

Ideally I would like to use the same way that std::sync::Arc uses (e.g., padding_needed_for), but that is unstable...
I will look into whether it is possible to emulate it in stable as part of #140.

EDIT: #141 changed it to use the same way as std::sync::Arc.

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.

portable_atomic_util::Arc::into_raw() returns unaligned reference to the inner type
3 participants