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.

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

Implement proper C ABI lowering for RISC-V #68452

Merged
merged 2 commits into from
Feb 8, 2020
Merged

Implement proper C ABI lowering for RISC-V #68452

merged 2 commits into from
Feb 8, 2020

Conversation

msizanoen1
Copy link
Contributor

@msizanoen1 msizanoen1 commented Jan 22, 2020 •
edited
Loading

This is necessary for full RISC-V psABI compliance when passing argument across C FFI boundary.

cc @lenary

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 22, 2020
@lenary
Copy link
Contributor

lenary commented Jan 22, 2020

Thanks for opening a pull request!

I would like to see codegen tests for each ABI (I would be content to only test the ABIs currently used by rustc), which are as comprehensive as the clang tests for the same. I'm happy to guide you to writing them, but they should be based on the tests in clang/test/CodeGen/riscv*-abi.c. This: https://github.com/llvm/llvm-project/blob/master/clang/test/CodeGen/riscv32-ilp32-abi.c for example, contains some of the tests for the ilp32 abi.

This will involve using structs annotated #[repr(C)] and functions annotated extern "C". There is guidance on adding tests in https://rust-lang.github.io/rustc-guide/tests/adding.html

@jonas-schievink jonas-schievink added the O-riscv Target: RISC-V architecture label Jan 22, 2020
@msizanoen1 msizanoen1 force-pushed the riscv-abi branch 4 times, most recently from cd71c24 to 796b21d Compare January 22, 2020 13:56
@msizanoen1

This comment has been minimized.

Sign in to view
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 23, 2020
@msizanoen1 msizanoen1 force-pushed the riscv-abi branch 2 times, most recently from a73e032 to bac26c4 Compare January 23, 2020 04:54
@msizanoen1
Copy link
Contributor Author

The implementation is now complete. Now the only remaining part is adding tests.

@msizanoen1 msizanoen1 force-pushed the riscv-abi branch 3 times, most recently from c8993cc to a7dea90 Compare January 23, 2020 10:31
@msizanoen1
Copy link
Contributor Author

@rustbot modify labels: +T-compiler

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 23, 2020
@lenary lenary mentioned this pull request Jan 23, 2020
Merged
@msizanoen1
Copy link
Contributor Author

msizanoen1 commented Jan 24, 2020 •
edited
Loading

@rustbot modify labels: -S-waiting-on-author +S-waiting-on-review
It's now ready for review. No test are added because the are currently no RISC-V test runners on CI yet

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 24, 2020
@msizanoen1 msizanoen1 changed the title Implement proper hard-float ABI lowering for RISC-V Implement proper C ABI lowering for RISC-V Jan 24, 2020
@msizanoen1 msizanoen1 mentioned this pull request Jan 24, 2020
Closed
@programmerjake
Copy link
Member

It's now ready for review. No test are added because the are currently no RISC-V test runners on CI yet.

Seems like a good idea to add a RISC-V test runner using QEMU or similar.

@lenary
Copy link
Contributor

lenary commented Feb 4, 2020

@msizanoen1 there were some structs in your tests (specifically DoubleCharChar) that were thought to be "interesting" enough cases to add to the general purpose tests to ensure all backends got them correct, including RISC-V.

@eddyb
Copy link
Member

eddyb commented Feb 4, 2020

r? @nagisa (see #68452 (review))

@rust-highfive rust-highfive assigned nagisa and unassigned eddyb Feb 4, 2020
@nagisa
Copy link
Member

nagisa commented Feb 4, 2020

@bors r=nagisa,eddyb

@bors
Copy link
Contributor

bors commented Feb 4, 2020

📌 Commit 3963387 has been approved by nagisa,eddyb

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 4, 2020
@bors
Copy link
Contributor

bors commented Feb 4, 2020

⌛ Testing commit 3963387 with merge 737fd80bb3fc446fc83d9030c01f3ad638c2bf31...

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-tools of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors
Copy link
Contributor

bors commented Feb 4, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 4, 2020
@msizanoen1
Copy link
Contributor Author

msizanoen1 commented Feb 8, 2020 •
edited
Loading

@nagisa @eddyb Can you retry merge? It looks like a spurious error:

2020-02-04T23:54:10.1096957Z     Updating crates.io index
2020-02-04T23:54:24.3224839Z  Downloading crates ...
2020-02-04T23:54:24.5236532Z warning: spurious network error (2 tries remaining): failed to get 200 response from `https://crates.io/api/v1/crates/getopts/0.2.21/download`, got 502
2020-02-04T23:54:24.5405630Z warning: spurious network error (1 tries remaining): failed to get 200 response from `https://crates.io/api/v1/crates/getopts/0.2.21/download`, got 502
2020-02-04T23:54:24.5896285Z error: failed to download from `https://crates.io/api/v1/crates/getopts/0.2.21/download`
2020-02-04T23:54:24.5897385Z 
2020-02-04T23:54:24.5898138Z Caused by:
2020-02-04T23:54:24.5898568Z   failed to get 200 response from `https://crates.io/api/v1/crates/getopts/0.2.21/download`, got 502

@eddyb
Copy link
Member

eddyb commented Feb 8, 2020

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 8, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 8, 2020
@Dylan-DPC
Rollup merge of rust-lang#68452 - msizanoen1:riscv-abi, r=nagisa,eddyb
76de8a6 
Implement proper C ABI lowering for RISC-V

This is necessary for full RISC-V psABI compliance when passing argument across C FFI boundary.

cc @lenary
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 8, 2020
Closed
@bors
Copy link
Contributor

bors commented Feb 8, 2020

⌛ Testing commit 3963387 with merge 07a34df...

bors added a commit that referenced this pull request Feb 8, 2020
@bors
Auto merge of #68452 - msizanoen1:riscv-abi, r=nagisa,eddyb
07a34df 
Implement proper C ABI lowering for RISC-V

This is necessary for full RISC-V psABI compliance when passing argument across C FFI boundary.

cc @lenary
@bors
Copy link
Contributor

bors commented Feb 8, 2020

☀️ Test successful - checks-azure
Approved by: nagisa,eddyb
Pushing 07a34df to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 8, 2020
@bors bors merged commit 3963387 into rust-lang:master Feb 8, 2020
@msizanoen1 msizanoen1 deleted the riscv-abi branch February 9, 2020 03:31
@lenary lenary mentioned this pull request Mar 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb approved these changes

Assignees

@nagisa nagisa

Labels
merged-by-bors This PR was explicitly merged by bors. O-riscv Target: RISC-V architecture S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.