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

Add support for the win64 calling convention #10527

Merged
merged 3 commits into from
Nov 20, 2013
Merged

Conversation

eholk
Copy link
Contributor

@eholk eholk commented Nov 17, 2013

This was needed to access UEFI boot services in my new Boot2Rust experiment.

I also realized that Rust functions declared as extern always use the C calling convention regardless of how they were declared, so this pull request fixes that as well.

@alexcrichton
Copy link
Member

It's unfortunate to not see any tests for this. From what it looks like, if I declare a rust function as extern "something not C" fn foo() and call it, it should segfault today, right? If so, can we add some tests to make sure that this doesn't regress?

@thestinger
Copy link
Contributor

@alexcrichton: I expect this could expose some functions with a weird calling convention from one crate and call them from another, but you'd actually need to find a difference between the calling conventions and it would be very unlikely that it would segfault (as it's not going to break any memory protection rules in 99.9% of cases). You could write one where it would read the wrong registers or wrong bit of the stack I guess.

@alexcrichton
Copy link
Member

I'm not sure that you can do this, but if you can write code which segfaults before this patch, then you definitely have a test case right there. I would like to at least explore the idea of adding a test before having no tests at all.

@thestinger
Copy link
Contributor

It probably causes a segmentation fault after reading an incorrect value as the parameter (a null pointer from a register that happens to be zero, maybe).

@thestinger
Copy link
Contributor

(temporarily removed r+, waiting for a test)

@emberian
Copy link
Member

An easier test would be finding things which are passed differently, and asserting that a parameter has a specific value.

@eholk
Copy link
Contributor Author

eholk commented Nov 18, 2013

I'm working on a test case now. I'm making a function foo that takes 4 integers and makes sure they come in the same order. The win64 calling convention and the normal rust calling convention use different registers for argument passing, so the numbers come in the wrong order if the caller and callee don't agree on the calling convention. The only tricky part is you have to split the caller and the callee into separate crates because otherwise the LLVM optimizer is too smart. I'll add this change once I get a chance to make sure the test actually works.

@eholk
Copy link
Contributor Author

eholk commented Nov 18, 2013

Alright, I added the test case. I confirmed on that the test fails with the change that just adds support for the win64 convention, and it passes with the second change.

@alexcrichton
Copy link
Member

Hm, I think that the tests may not pass the make check target, could you add the license block to the top of the files?

Otherwise, thanks! Looks good to me!

@eholk
Copy link
Contributor Author

eholk commented Nov 18, 2013

I added the copyright header. Surprisingly, I don't think make check checked for this.

I did get a rustpkg test failure about rebuilding a C file, but I think that is unrelated to this change.

@alexcrichton
Copy link
Member

Could you also squash these commits into the first one being the win64 ABI/test case, and then the second is the ABI fix in general?

@brson
Copy link
Contributor

brson commented Nov 18, 2013

Make tidy indeed doesn't currently check test cases: #4534.

@eholk
Copy link
Contributor Author

eholk commented Nov 19, 2013

Ok, I squashed these down, rebased onto the latest master and made sure make check passes.

@eholk
Copy link
Contributor Author

eholk commented Nov 20, 2013

@alexcrichton - would you might reviewing my latest addition? The tests failed on the 32-bit machines, so I've just substituted the normal calling convention there.

bors added a commit that referenced this pull request Nov 20, 2013
This was needed to access UEFI boot services in my new Boot2Rust experiment.

I also realized that Rust functions declared as extern always use the C calling convention regardless of how they were declared, so this pull request fixes that as well.
@bors bors closed this Nov 20, 2013
@bors bors merged commit 02e565a into rust-lang:master Nov 20, 2013
@eholk eholk deleted the win64 branch November 20, 2013 20:12
flip1995 pushed a commit to flip1995/rust that referenced this pull request Mar 24, 2023
Do not propose to simplify a not expression coming from a macro

Fixes rust-lang#10523

changelog: FP [`nonminimal_bool`]: do not propose to change code coming from a macro
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.

6 participants