Skip to content

make native and crust functions conform to x86-64 ABI #1970

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

Closed
wants to merge 1 commit into from

Conversation

crabtw
Copy link
Contributor

@crabtw crabtw commented Mar 13, 2012

  • add functions to classify argument and return types
  • convert argument and return types of native function in shim function
  • convert argument and return types of crust function in wrap function

@brson
Copy link
Contributor

brson commented Mar 14, 2012

Thank you! This looks really interesting, but I haven't looked at it closely yet.

I would like some tests, though I realize there's not a way to test this in our current test suite. One thing you could do, that I have done before and would be fine with here, is add some functions to the runtime, possibly under src/rt/rust_test_x86_64.cpp that exhibit some of the problems this is addressing, and prefix them rust_dbg_. Then exercise those functions in some tests.

@brson
Copy link
Contributor

brson commented Mar 14, 2012

Does this completely fix #1402?

@crabtw
Copy link
Contributor Author

crabtw commented Mar 15, 2012

Yes, it fixes #1402 .
I will add some tests and move ABI dependent code to another module to make it more extensible.

@graydon
Copy link
Contributor

graydon commented Mar 16, 2012

This is fantastic! Thank you so much.

My only concerns are, as you say, modularizing it a bit (per-arch) and making some tests. But I'm happy to do that work myself while integrating it. You did the hard part. Let me know if you prefer to do that yourself or have me do it. It's fine either way.

@crabtw
Copy link
Contributor Author

crabtw commented Mar 16, 2012

I think you can do it better than me, so please do it.
Thank you!

@ghost ghost assigned graydon Mar 20, 2012
@graydon
Copy link
Contributor

graydon commented Mar 20, 2012

Actually I'm going to land this first and then modularize and test in subsequent steps. Assuming it bootstraps; we do need this sort of code in one form or another.

@graydon
Copy link
Contributor

graydon commented Mar 20, 2012

This is now integrated, thanks! I'm going to leave the issue just a little while longer to remind me to write tests. Modularizing can come later; I just want to make sure it fixes #1402.

@graydon graydon closed this in 855c99e Mar 20, 2012
@graydon graydon reopened this Mar 21, 2012
@graydon
Copy link
Contributor

graydon commented Mar 21, 2012

Fails testcase I threw together. I'll start debugging. Reopening.

@graydon
Copy link
Contributor

graydon commented Mar 21, 2012

Oh, seems to fail on x86 not x86_64. I think probably that path was broken before, then. I'll file a separate bug on just that case and re-close this. Thanks.

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.

3 participants