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

check target abi support #36421

Merged
merged 2 commits into from
Oct 26, 2016
Merged

check target abi support #36421

merged 2 commits into from
Oct 26, 2016

Conversation

TimNN
Copy link
Contributor

@TimNN TimNN commented Sep 12, 2016

This PR checks for each extern function / block whether the ABI / calling convention used is supported by the current target.

This was achieved by adding an abi_blacklist field to the target specifications, listing the calling conventions unsupported for that target.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@TimNN
Copy link
Contributor Author

TimNN commented Sep 13, 2016

cc @alexcrichton - I think you are one of the "experts" of the target related stuff.

@alexcrichton
Copy link
Member

Thanks for the PR! Is there an associated issue with this as well? I'd naively expect that all ABIs are supported everywhere (e.g. they're just register definitions), but I'm likely missing something...

@TimNN
Copy link
Contributor Author

TimNN commented Sep 13, 2016

There are multiple (related) issues: #36142, #36348, #24958

Summary: LLVM can assert (or apparently even segfault) when encountering unsupported calling conventions.

@alexcrichton
Copy link
Member

Ok, sounds good to me! The strategy here looks good to me as well in that case.

We want to be sure to not regress anything as well (unfortunately crater won't be much help here), but perhaps this example could be used to figure out which calling conventions are allowed on which platforms?

@TimNN
Copy link
Contributor Author

TimNN commented Sep 16, 2016

Alright so I did a bit of testing:

Test sources and script are here: https://gist.github.com/0582b8a1f9a1f35cbf06a8a144b82bf2

The results on OS X with nightly-2016-09-15 are also here: https://docs.google.com/spreadsheets/d/1LImcJbNuLOEjfzxsCOI1dYzwkpp7guLAQTb48jq1YLY/edit

Summary:

  • arm only supports cdecl and aapcs
  • vectorcall llvm asserts on i586 windows targets (in getFPReg)
  • win64 and sysv64 llvm assert on i368-apple-ios and i686-apple-darwin (in getLLVMRegNum)
  • asmjs and nacl fail with "no vaialable targets are compatible with this triple" (probably expected)
  • mips64 targets ICE with data layout mismatch (with LLVM default)
  • powerpc64-unknown-linux-gnu ICEs (even for the reference "C" abi) with 'assertion failed:(left == right)(left:Indirect, right:Direct)', ../src/librustc_trans/abi.rs:100

Conclusions:

  • Allow only cdecl and aapcs on ARM
  • The mips64 stuff should probably be fixed by just updating our datalayout?
  • The powerpc64-unknown-linux-gnu should probably be fixed as well
  • No idea about asmjs or nacl
  • No idea about the other "llvm assert"s, these are probably upstream bugs?

@alexcrichton
Copy link
Member

Awesome, thanks for the investigation! That all looks great to me. We basically just don't want to break anyone, so whether it's an assert or a legit error it should be fine to ignore. Want to use that as a whitelist for figuring out which platforms can use which ABI?

@bors
Copy link
Contributor

bors commented Sep 29, 2016

☔ The latest upstream changes (presumably #36818) made this pull request unmergeable. Please resolve the merge conflicts.

@TimNN TimNN force-pushed the check-abis branch 4 times, most recently from 05737b7 to ff2a6c6 Compare October 24, 2016 12:54
@TimNN
Copy link
Contributor Author

TimNN commented Oct 24, 2016

So I (finally!) updated this PR with the following changes:

@TimNN TimNN changed the title WIP: check target abi support check target abi support Oct 24, 2016
@alexcrichton
Copy link
Member

@bors: r+

Awesome, thanks @TimNN!

@bors
Copy link
Contributor

bors commented Oct 25, 2016

📌 Commit 9eb0fd9 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Oct 25, 2016

⌛ Testing commit 9eb0fd9 with merge f542ad1...

@bors
Copy link
Contributor

bors commented Oct 25, 2016

💔 Test failed - auto-linux-64-x-android-t

@TimNN
Copy link
Contributor Author

TimNN commented Oct 25, 2016

I updated the tests, hope I've got them all.

But just to check, // ignore-arm ignores armv7 stuff as well, does it (in compiletest)?

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 25, 2016

📌 Commit 1422ac9 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Oct 26, 2016

⌛ Testing commit 1422ac9 with merge 586a988...

bors added a commit that referenced this pull request Oct 26, 2016
check target abi support

This PR checks for each extern function / block whether the ABI / calling convention used is supported by the current target.

This was achieved by adding an `abi_blacklist` field to the target specifications, listing the calling conventions unsupported for that target.
@bors bors merged commit 1422ac9 into rust-lang:master Oct 26, 2016
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.

5 participants