Skip to content

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 <[email protected]>
Signed-off-by: John Nunley <[email protected]>
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
@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.

@taiki-e taiki-e added the A-portable-atomic-util Area: related to portable-atomic-util crate label Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-portable-atomic-util Area: related to portable-atomic-util crate
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