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

Fix layout differences of Header<Atomic> v.s. Header<NonAtomic> #59

Merged
merged 1 commit into from
Feb 10, 2022
Merged

Fix layout differences of Header<Atomic> v.s. Header<NonAtomic> #59

merged 1 commit into from
Feb 10, 2022

Conversation

SimonSapin
Copy link
Member

This fixes #58, which shows UB when converting between non-atomic and atomic Tendril. This conversion is exposed without unsafe through SendTendril and implemented with transmute, so matching layout is essential.

@SimonSapin
Copy link
Member Author

r? @jdm

@jdm
Copy link
Member

jdm commented Jan 28, 2022

CI is failing on nightly while testing the C API. Should we just rip it out?

@SimonSapin
Copy link
Member Author

Yes. I thought we had already.

@KamilaBorowska KamilaBorowska mentioned this pull request Feb 10, 2022
Merged
@KamilaBorowska
Copy link
Contributor

struct Header should be changed to be #[repr(C)], Rust won't otherwise guarantee that Header<Atomic> and Header<NonAtomic> have fields in the same order. Currently UCG doesn't guarantee deterministic layout (rust-lang/unsafe-code-guidelines#35).

bors-servo added a commit that referenced this pull request Feb 10, 2022
@bors-servo
Auto merge of #61 - xfix:remove-capi, r=SimonSapin
cd133fe 
Remove C API

I think it may be necessary for #59.

Note that because of `transmute` tricks `#[repr(C)]` has to stay to ensure stable representation.
@SimonSapin
Fix layout differences of Header<Atomic> v.s. Header<NonAtomic>
bdd07bf 
This fixes #58, which shows UB
when converting between non-atomic and atomic `Tendril`.
This conversion is exposed without `unsafe` through `SendTendril`
and implemented with `transmute`, so matching layout is essential.
@SimonSapin
Copy link
Member Author

Rebased on top of #61, and added #[repr(C)]

@jdm
Copy link
Member

jdm commented Feb 10, 2022

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit bdd07bf has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit bdd07bf with merge 3df7383...

@bors-servo
Copy link
Contributor

☀️ Test successful - checks-github
Approved by: jdm
Pushing 3df7383 to master...

@bors-servo bors-servo merged commit 3df7383 into master Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm jdm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Changing Tendril Atomicity with SendTendril produces garbage.
4 participants
@SimonSapin @jdm @KamilaBorowska @bors-servo