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

Swap size and value args in roundtrip tests #41

Merged
merged 1 commit into from
Oct 14, 2022

Conversation

wesleywiser
Copy link
Contributor

@wesleywiser wesleywiser commented Aug 26, 2022

If there is a disagreement on the size of the value, this can cause
issues on archs that pass parameters on the stack (such as x86) leading
to non-sensical errors like this:

size of struct sched_param is 28 in C and -134597808 in Rust

Swapping the parameter order alleviates this:

size of struct sched_param is 28 in C and 20 in Rust

If there is a disagreement on the size of the value, this can cause
issues on archs that pass parameters on the stack (such as x86) leading
to non-sensical errors like this:

> size of struct sched_param is 28 in C and -134597808 in Rust
@JohnTitor
Copy link
Owner

Is there any way to test it? Does it work to add a test code to testcrate and run it on i686-unknown-linux-gnu?

@JohnTitor
Copy link
Owner

JohnTitor commented Oct 14, 2022

@wesleywiser Friendly-ping, do you have any thoughts about the above comment? Note that the testcrate currently doesn't test items correctly (see #45), so it's fine to accept w/o a test :)

@wesleywiser
Copy link
Contributor Author

Sorry! I took a look briefly the other week but couldn't quite figure out how to get a good testcase and haven't had much time to look at it again since.

Copy link
Owner

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

Alright, then I'm going to merge as-is. Thanks for contributing and investigating!

@JohnTitor JohnTitor merged commit f419830 into JohnTitor:master Oct 14, 2022
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.

None yet

2 participants