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

rustc: Start a custom cabi module for wasm32 #48959

Merged
merged 1 commit into from
Mar 16, 2018

Conversation

alexcrichton
Copy link
Member

It actually was already using the cabi_asmjs module but that was by accident,
so route the new wasm32-unknown-unknown target to a new cabi_wasm32 module.
The first entries in this module are to use signext and zeroext for types
that are under 32 bytes in size

Closes rust-lang-nursery/rust-wasm#88

It actually was already using the `cabi_asmjs` module but that was by accident,
so route the new `wasm32-unknown-unknown` target to a new `cabi_wasm32` module.
The first entries in this module are to use `signext` and `zeroext` for types
that are under 32 bytes in size

Closes rust-lang-nursery/rust-wasm#88
@rust-highfive
Copy link
Collaborator

r? @cramertj

(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 Mar 12, 2018
@alexcrichton
Copy link
Member Author

r? @eddyb

I think this is the first time I've attempted to manufacture new cabi code from scratch, so it likely isn't right! (the current state is modeled after asmjs's)

@eddyb
Copy link
Member

eddyb commented Mar 15, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Mar 15, 2018

📌 Commit 74f5dd0 has been approved by 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 Mar 15, 2018

fn classify_ret_ty<'a, 'tcx>(_cx: &CodegenCx<'a, 'tcx>, ret: &mut ArgType<'tcx>) {
ret.extend_integer_width_to(32);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't do the aggregate to indirect conversion anymore that the asm.js C ABI did: https://github.com/alexcrichton/rust/blob/74f5dd07cff8dba348f91cfef4df5242ab33a311/src/librustc_trans/cabi_asmjs.rs#L20-L33

However looking at how the C ABI for wasm is implemented in clang, it seems to still do it: https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/TargetInfo.cpp#L759-L775

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yeah TBH I have very little clue what ABI-related code is doing...

Seems fine though for us to match clang! Until there's an "official ABI" though this'll probably be a game of cat and mouse.

Copy link
Contributor

@CryZe CryZe Mar 15, 2018

Choose a reason for hiding this comment

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

"Seems fine though for us to match clang!"
I'm a bit confused by that wording. What I'm saying is that we don't match clang anymore with this PR, so shouldn't we include the aggregate code as well to properly match it again?

It seems to be the same as the asm.js one, so we can just copy the code from there / call it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Er sorry, what I mean is that it seems fine to add. It won't have any effect today anyway as you can't intermingle wasm32-unknown-unknown and C yet so I'm hoping that we can fix this with a test in the future. (I'm also hesitant to add things that I don't personally understand in my own PRs)

Copy link
Contributor

@CryZe CryZe Mar 15, 2018

Choose a reason for hiding this comment

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

Well it also affects everyone who is calling an exported function from JS / a wasm environment. However I guess we don't really give any guarantees how structs are represented there anyway, so I guess it's fine to wait on the C ABI settling.

Maybe we should open an issue about this though? Otherwise we might forget about this disparity.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah with a lack of a spec about an ABI returning any sort of aggregate or non-integer/float is sort of "best effort" today anyway

@CryZe
Copy link
Contributor

CryZe commented Mar 15, 2018

Also I have a question. Does this only affect the C ABI or also the Rust ABI? Cause afaict the Rust ABI doesn't need it, and it would probably have a performance impact if it did.

@alexcrichton
Copy link
Member Author

@CryZe AFAIK the files here only affect the C ABI, not internal Rust functions

@alexcrichton
Copy link
Member Author

@bors: rollup

kennytm added a commit to kennytm/rust that referenced this pull request Mar 15, 2018
rustc: Start a custom cabi module for wasm32

It actually was already using the `cabi_asmjs` module but that was by accident,
so route the new `wasm32-unknown-unknown` target to a new `cabi_wasm32` module.
The first entries in this module are to use `signext` and `zeroext` for types
that are under 32 bytes in size

Closes rust-lang-nursery/rust-wasm#88
bors added a commit that referenced this pull request Mar 16, 2018
Rollup of 17 pull requests

- Successful merges: #48706, #48875, #48892, #48922, #48957, #48959, #48961, #48965, #49007, #49024, #49042, #49050, #48853, #48990, #49037, #49049, #48972
- Failed merges:
@bors bors merged commit 74f5dd0 into rust-lang:master Mar 16, 2018
@alexcrichton alexcrichton deleted the signext branch March 16, 2018 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants