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

Transfer the packed attribute on unions to repr(packed) #347

Merged
merged 2 commits into from
Jan 18, 2022

Conversation

chrysn
Copy link
Contributor

@chrysn chrysn commented Aug 27, 2021

Closes: #346


This should be a straightforward solution to #346, and works for me on RIOT (which also means I can't easily test it on the main branch, as I still need my for-riot vendor branch that has #311 and the branch mentioned in #309 merged).

There are no tests yet; is

$ cargo +nightly-20109-12-05 build --locked --release
$  python3 scripts/test_translator.py ./tests

the right procedure?

@chrysn
Copy link
Contributor Author

chrysn commented Aug 27, 2021

CI seems broken; the above tests ran through, and now also contain a test for the new code. I've also run the tests against an old build; these fail as is characteristic for a good test. (The unpacked union would be of size 8 instead of 5).

Oh, one more remark on this PR: All nontrivial code is lifted from structs over to unions, not necessarily understanding every detail. It does work, and all the copied code looks unsuspicious, but still I'd like to point that out for reviewers. (There's probably an opportunity for some refactoring, but I'd like to keep that separate from the changes, especially as there already is code duplicated in this style).

@thedataking
Copy link
Contributor

@chrysn the Darwin failures were due to timeouts as far as I can tell. I've opened a PR to get CI working properly again.

@chrysn
Copy link
Contributor Author

chrysn commented Jan 13, 2022

Is there anything that should be addressed about this PR? Does it help if I rebase it onto current master?

@thedataking
Copy link
Contributor

LTGM but I'm no expert on packed unions so I'd like a second pair of eyes before we merge. And yes, rebasing onto current master would be nice.

@chrysn
Copy link
Contributor Author

chrysn commented Jan 15, 2022 via email

@chrysn
Copy link
Contributor Author

chrysn commented Jan 16, 2022

I've given the rebased packed-unions a test run. Most tests also merged in the minimal-309 branch referenced in #309 (comment), as that's necessary for other parts of the code. As expected, that branch has passed the inline casting tests (asserting that bindgen's view of the C structs is as large as C2Rust's), which current master plus minimal-309 does not).

Copy link
Contributor

@fw-immunant fw-immunant left a comment

Choose a reason for hiding this comment

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

This LGTM and does what it says on the tin. I think we should merge.

@fw-immunant
Copy link
Contributor

I think it's worth noting separately, however, that we still have some holes in our support of the packed attribute--it isn't respected yet for struct or union fields, and somewhat relatedly we still have #136 outstanding. In docs/known-limitations.md we mention missing GNU packed struct support--this should probably be clarified to describe exactly which meanings/contexts of __attribute__((packed)) we support.

@fw-immunant fw-immunant merged commit 6674d78 into immunant:master Jan 18, 2022
@chrysn chrysn deleted the packed-unions branch January 18, 2022 17:38
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.

packed attribute not transferred on unions
3 participants