Skip to content

SystemV struct arguments #1559

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 17, 2020
Merged

SystemV struct arguments #1559

merged 5 commits into from
Jul 17, 2020

Conversation

bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Apr 20, 2020 •
edited
Loading

According to the SystemV abi, struct arguments must be passed at a fixed stack offset. Cranelift didn't have any way to implement this before. This is necessary to compile proc-macros using cg_clif: https://github.com/bjorn3/rustc_codegen_cranelift/issues/905#issuecomment-616173297

Fixes #1108

TODO

  • Don't misuse INVALID, but introduce a proper type.
  • Implement this correctly for WindowsFastcall or give an error.

@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Apr 20, 2020
@github-actions
Copy link

Subscribe to Label Action

cc @bnjbvr

This issue or pull request has been labeled: "cranelift"

Thus the following users have been cc'd because of the following labels:

  • bnjbvr: cranelift

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

This was referenced Apr 20, 2020
Closed
Closed
Closed
@alexcrichton alexcrichton changed the base branch from master to main June 25, 2020 18:50
@bjorn3 bjorn3 force-pushed the abi_struct_args branch from 2d60da4 to 9ba77ed Compare June 27, 2020 14:32
@github-actions github-actions bot added the cranelift:meta Everything related to the meta-language. label Jun 27, 2020
@bjorn3
Copy link
Contributor Author

bjorn3 commented Jun 27, 2020

Ready for review. There are two hacks that can be removed once the old style backends are removed: The __sarg type and the __sarg_dummy instruction.

@bjorn3 bjorn3 marked this pull request as ready for review June 27, 2020 15:26
@bjorn3
Copy link
Contributor Author

bjorn3 commented Jun 27, 2020

r? @fitzgen

@github-actions github-actions bot added the cranelift:area:aarch64 Issues related to AArch64 backend. label Jun 27, 2020
@bjorn3 bjorn3 force-pushed the abi_struct_args branch 3 times, most recently from 26cf5fe to abe0381 Compare June 27, 2020 19:44
@bjorn3
Copy link
Contributor Author

bjorn3 commented Jun 28, 2020

All tests pass.

@bjorn3 bjorn3 changed the title [WIP] SystemV struct arguments SystemV struct arguments Jun 28, 2020
@bjorn3 bjorn3 mentioned this pull request Jun 29, 2020
Closed
14 tasks
@fitzgen
Copy link
Member

fitzgen commented Jun 29, 2020

Are the TODOs in the original comment taken care of?

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jun 29, 2020

Are the TODOs in the original comment taken care of?

Thanks for reminding me.

Don't misuse INVALID, but introduce a proper type.

I have introduced the SARG__ type.

Implement this correctly for WindowsFastcall or give an error.

I will add an error for now.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jun 29, 2020

I have turned it into a panic, as there is no way to report an error in the function signature legalization.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jun 29, 2020

Should I squash?

@fitzgen
Copy link
Member

fitzgen commented Jun 29, 2020

That would be helpful, yes.

@bjorn3 bjorn3 force-pushed the abi_struct_args branch from da3e0ae to 53b05d8 Compare June 29, 2020 17:30
@bjorn3
Copy link
Contributor Author

bjorn3 commented Jun 29, 2020

Splat

@bjorn3 bjorn3 force-pushed the abi_struct_args branch from 53b05d8 to c882d00 Compare June 29, 2020 17:46
@bjorn3 bjorn3 force-pushed the abi_struct_args branch from c882d00 to 8fbde92 Compare June 29, 2020 18:53
@bjorn3
Copy link
Contributor Author

bjorn3 commented Jun 29, 2020

CI passed

@fitzgen
Copy link
Member

fitzgen commented Jun 29, 2020

Thanks for rebasing @bjorn3.

I took a look at the code, but I don't feel familiar enough with many of the bits that it is touching to be the official reviewer, determine if this is the proper approach to solving the problem, etc. I think @bnjbvr or @sunfishcode would be better reviewers.

@tschneidereit tschneidereit requested review from bnjbvr and cfallin July 15, 2020 10:54
cfallin
cfallin reviewed Jul 15, 2020
View reviewed changes
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

This looks fine to me overall, with the caveat that I am not intimately familiar with details of the old ABI mechanisms. (Given that we'll be moving over to the new backend at some point, I am not too worried about perfecting the design, though.) Just a few nits below. Thanks!

@cfallin
Copy link
Member

cfallin commented Jul 15, 2020

Also, to be clear, like others above I'd also be more comfortable if someone who's worked more on the old backend could review as well.

@bjorn3 bjorn3 force-pushed the abi_struct_args branch from b70f84c to 8cd6d15 Compare July 15, 2020 19:00
@cfallin
Copy link
Member

cfallin commented Jul 15, 2020

@bnjbvr , would you mind sanity-checking this? LGTM from code-quality perspective, just want to make sure I'm not missing anything w.r.t. interactions with the old ABI mechanisms.

@bjorn3 bjorn3 force-pushed the abi_struct_args branch from 8cd6d15 to abfe2b6 Compare July 15, 2020 19:23
bnjbvr
bnjbvr approved these changes Jul 16, 2020
View reviewed changes
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Seems legit, but I can't exactly recall the mechanics of argument passing in the old backend. So as long as it is well tested (I added suggestions below), lgtm.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jul 16, 2020

macOS doesn't have any logs for the failure.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Jul 17, 2020

It passed this time.

@bnjbvr bnjbvr merged commit 5c5a30f into bytecodealliance:main Jul 17, 2020
@bjorn3 bjorn3 deleted the abi_struct_args branch July 17, 2020 10:09
@bjorn3
Copy link
Contributor Author

bjorn3 commented Jul 17, 2020

Thanks!

@bjorn3 bjorn3 mentioned this pull request Jul 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfallin cfallin cfallin left review comments

@bnjbvr bnjbvr bnjbvr approved these changes

Assignees
No one assigned
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

How to call functions where args must be passed at the stack
4 participants
@bjorn3 @fitzgen @cfallin @bnjbvr