Skip to content

Fix the asmjs C ABI #31653

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

Merged
merged 1 commit into from
Feb 15, 2016
Merged

Fix the asmjs C ABI #31653

merged 1 commit into from
Feb 15, 2016

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Feb 14, 2016

Needs a correct review because I'm not too confident with how this works.
All tests related to the C ABI are now passing.

References:

The classifyArgumentType function has two different paths depending on RAA == CGCXXABI::RAA_DirectInMemory, but I don't really know what's the corresponding option in Rust.

cc @brson @eddyb

@rust-highfive
Copy link
Contributor

r? @alexcrichton

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

@tomaka tomaka changed the title Fix the asmjs ABI Fix the asmjs C ABI Feb 14, 2016
@tomaka
Copy link
Contributor Author

tomaka commented Feb 14, 2016

The two URLs I pasted in the source code don't pass make tidy. Am I supposed to split them in multiple lines?

@eddyb
Copy link
Member

eddyb commented Feb 14, 2016

I think you should use relative paths into the repo and maybe put a link to the whole repo on a different line.

@alexcrichton
Copy link
Member

I'll be honest in that I have no idea what these files do, they're entirely black boxes to me, perhaps, though:

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned alexcrichton Feb 14, 2016
ArgType::indirect(ty, Some(Attribute::StructRet))
},
_ => {
let attr = if ty == Type::i1(ccx) { Some(Attribute::ZExt) } else { None };
Copy link
Member

Choose a reason for hiding this comment

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

i8 and others don't get extended?

@eddyb
Copy link
Member

eddyb commented Feb 14, 2016

This is good progress IMO, @bors r+

@bors
Copy link
Collaborator

bors commented Feb 14, 2016

📌 Commit 5b224ec has been approved by eddyb

@bors
Copy link
Collaborator

bors commented Feb 15, 2016

⌛ Testing commit 5b224ec with merge 9a79137...

bors added a commit that referenced this pull request Feb 15, 2016
Needs a correct review because I'm not too confident with how this works.
All tests related to the C ABI are now passing.

References:
- https://github.com/kripken/emscripten-fastcomp-clang/blob/dbe68fecd03d6f646bd075963c3cc0e7130e5767/lib/CodeGen/TargetInfo.cpp#L479-L489
- https://github.com/kripken/emscripten-fastcomp-clang/blob/dbe68fecd03d6f646bd075963c3cc0e7130e5767/lib/CodeGen/TargetInfo.cpp#L466-L477

The `classifyArgumentType` function has two different paths depending on `RAA == CGCXXABI::RAA_DirectInMemory`, but I don't really know what's the corresponding option in Rust.

cc @brson @eddyb
@bors bors merged commit 5b224ec into rust-lang:master Feb 15, 2016
@tomaka tomaka deleted the emscripten-abi branch February 15, 2016 14:04
@brson
Copy link
Contributor

brson commented Feb 19, 2016

Neat!

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.

6 participants