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

Generate layout tests using real sizeof/alignof #181

Merged
merged 13 commits into from
May 17, 2024

Conversation

ian-h-chamberlain
Copy link
Member

@ian-h-chamberlain ian-h-chamberlain commented Apr 23, 2024

Similar to the tests generated by bindgen, but using our actual toolchain to spit out the expected values instead of what libclang generates (bindgen's default behavior).

The full generated test can be downloaded from Actions: https://github.com/rust3ds/ctru-rs/actions/runs/9072691412/artifacts/1499437252

Here's a partial example:

#[test]
fn layout_test_tag_aptHookCookie() {
    assert_eq!(
        size_of!(tag_aptHookCookie),
        cpp ! (unsafe [] -> usize as "size_t" { return sizeof (tag_aptHookCookie) ; }),
        "{} != {}",
        stringify!(size_of!(tag_aptHookCookie)),
        stringify!(sizeof(tag_aptHookCookie)),
    );
    assert_eq!(
        align_of!(tag_aptHookCookie),
        cpp ! (unsafe [] -> usize as "size_t" { return alignof (tag_aptHookCookie) ; }),
        "{} != {}",
        stringify!(align_of!(tag_aptHookCookie)),
        stringify!(alignof(tag_aptHookCookie)),
    );
    assert_eq!(
        size_of!(tag_aptHookCookie::callback),
        cpp ! (unsafe [] -> usize as "size_t" { return sizeof (tag_aptHookCookie :: callback) ; }),
        "{} != {}",
        stringify!(size_of!(tag_aptHookCookie::callback)),
        stringify!(sizeof(tag_aptHookCookie::callback)),
    );
    assert_eq!(
        align_of!(tag_aptHookCookie::callback),
        cpp ! (unsafe [] -> usize as "size_t" { return alignof (tag_aptHookCookie :: callback) ; }),
        "{} != {}",
        stringify!(align_of!(tag_aptHookCookie::callback)),
        stringify!(alignof(tag_aptHookCookie::callback)),
    );
    assert_eq!(
        offset_of!(tag_aptHookCookie, callback),
        cpp ! (unsafe [] -> usize as "size_t" { return offsetof (tag_aptHookCookie , callback) ; }),
        "{} != {}",
        stringify!(offset_of!(tag_aptHookCookie, callback)),
        stringify!(offsetof(tag_aptHookCookie, callback)),
    );
}

Base automatically changed from fix/bindings-short-enums to master April 23, 2024 02:57
@ian-h-chamberlain ian-h-chamberlain force-pushed the feature/cpp-sizeof-tests branch 2 times, most recently from d47c585 to d658291 Compare May 6, 2024 03:20
@ian-h-chamberlain ian-h-chamberlain marked this pull request as ready for review May 14, 2024 02:04
@ian-h-chamberlain ian-h-chamberlain requested a review from a team as a code owner May 14, 2024 02:04
Copy link
Member

@Meziu Meziu left a comment

Choose a reason for hiding this comment

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

A very interesting solution for our custom-compiler setup! This might just solve all size-related uncertainties in the testing suite.

Other than for a couple of missing features, which aren't really needed, I feel like this addition has all that it needs to be an useful replacement for bindgen tests. Great job! 👏

Cargo.toml Outdated Show resolved Hide resolved
cc = "1.0"
cpp_build = { optional = true, git = "https://github.com/mystor/rust-cpp.git" }
Copy link
Member

Choose a reason for hiding this comment

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

Why not use the version on crates.io?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly so I could use mystor/rust-cpp#111 which hasn't been released yet. We could do without it and just re-define all the cc configuration, or pin the commit hash, but I wasn't too worried about it since this is a test-only dependency

ctru-sys/Cargo.toml Outdated Show resolved Hide resolved
Blocklist specific bitfields etc instead of whole types, and document
why each one is blocklisted. Also support field renames for mangling
(e.g. `type`).
@ian-h-chamberlain
Copy link
Member Author

Other than for a couple of missing features, which aren't really needed, I feel like this addition has all that it needs to be an useful replacement for bindgen tests. Great job! 👏

This is an interesting point — for now, I've left the bindgen tests in place because I think they might still be useful to assert properties that clang thinks about the generated types... Especially given that only adds around 1 minute to CI time, and takes almost no effort on our part, do you think we should leave those alone and keep running them alongside these new tests?

Otherwise, I think this PR is pretty much good to go so I will probably merge once everything is passing 🚀

@Meziu
Copy link
Member

Meziu commented May 17, 2024

This is an interesting point — for now, I've left the bindgen tests in place because I think they might still be useful to assert properties that clang thinks about the generated types... Especially given that only adds around 1 minute to CI time, and takes almost no effort on our part, do you think we should leave those alone and keep running them alongside these new tests?

Very much so, bindgen tests are still a pretty integral part in the binding generation routine, and they are still the "best" comparative case for the suite added here. I'd keep them around for as long as we feel fit for testing the new suite and tweaking the bindgen setup (which after all these past issues I don't trust to be precise enough 😅).

Otherwise, I think this PR is pretty much good to go so I will probably merge once everything is passing 🚀

👍

@ian-h-chamberlain ian-h-chamberlain merged commit 2d25f88 into master May 17, 2024
4 checks passed
@ian-h-chamberlain ian-h-chamberlain deleted the feature/cpp-sizeof-tests branch May 17, 2024 17:54
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.

2 participants