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

Add test support for emscripten targets #610

Closed
wants to merge 22 commits into from

Conversation

malbarbo
Copy link
Contributor

@malbarbo malbarbo commented Jun 8, 2017

Also fix some definitions to make the tests pass for asmjs-unknown-emscripten. The build fails for wasm-unknown-emscripten with an assert error.

@malbarbo
Copy link
Contributor Author

malbarbo commented Jun 8, 2017

I'm waiting the build fails for wasm-unknown-emscripten so I can open a issue and link the error.

@malbarbo
Copy link
Contributor Author

malbarbo commented Jun 9, 2017

This failure is expected, I will disable wasm test an report the assert faillure to emscripten.

Edit: issue report.

@malbarbo
Copy link
Contributor Author

malbarbo commented Jun 9, 2017

Sadly, this changes will limit the file size that can be addressed to 2GB, but this is emscripten issue... It seems that all *64_t types and *64 functions related with file system are alias to they "normal" version...

@alexcrichton
Copy link
Member

This looks awesome, thanks! Would it be possible to avoid the large whitelist of functions in libc-test/build.rs? could those functions be pushed down a layer perhaps?

@malbarbo
Copy link
Contributor Author

could those functions be pushed down a layer perhaps?

I think this will make libstd (and others crates) more difficult to maintain. Take for example the fork function, it is not available on emscripten (it is in the whitelist) but it is used in libstd. An user will only get an error if he tries to use a function that calls fork, but he can use all the others functions. This is the same approach used in emscripten, that also defines the prototype of some functions but does not provides an implementation.

I don't know how many of the whitelisted functions are used in libstd, but I think removing them from libc would make it impossible to make libstd compile for emscripten.

@alexcrichton
Copy link
Member

Oh ok in that case mind clarifying something? So does fork exist in emscripten? If it does then how come tests are failing? If not, how does libstd link correctly?

@malbarbo
Copy link
Contributor Author

So, now I'm confused. It toke me some time to create the whitelist, and now I think I need to review it. fork and others functions can be removed and tests still pass, but the majority of the functions are still there.

Some functions, for example aio_*, are compiled ok when used in all.c, but fails when linking libc-test. I think that all.c is only compiled and not linked, and aio_* prototypes are defined in emscripten headers, so it works. But when libc-test are linked, the symbols are not found and the linking fails.

Now I don't know what is the best approach here. If we remove these definitions, a library that uses them cannot be compiled, and must cfg_out functions that use these definitions. On the other hand, if we leave it, the library can be compiled but will fail to link (in case an unimplemented function is necessary, but I think it will link ok if no such a function is used).

@malbarbo malbarbo force-pushed the emscripten branch 2 times, most recently from d30caf0 to 1ea20cb Compare June 13, 2017 20:44
@lilianmoraru
Copy link

emsdk install sdk-1.37.14-64bit doesn't work yet(of course, it was tagged ~30 minutes ago).
I'll be back here to continue spamming the issue when it will be available 😄

@lilianmoraru
Copy link

The SDKs are built and stored on https://s3.amazonaws.com/mozilla-games/emscripten/.
Searched for mozilla-games but didn't find much. No idea who does the builds...

@malbarbo
Copy link
Contributor Author

Update the emscripten sdk, let's see how travis goes. I didn't work on the whitelist issue.

@lilianmoraru
Copy link

The wasm build passed.
@malbarbo, this is fantastic!!
Thanks for moving this forward.

@alexcrichton
Copy link
Member

Awesome, thanks @malbarbo!

My only hesitation is the large whitelist of functions to ignore in libc-test/build.rs. Does libstd use these functions? If so, how come we can't check them? If not, can they be omitted on emscripten? (presumably they don't exist)

@bors
Copy link
Contributor

bors commented Jul 7, 2017

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

@lilianmoraru
Copy link

@alexcrichton What needs to be done(finished-up) here to push this forward?
Proper emscripten and wasm support in Rust is basically blocked by this and it would be great to have this in.

@alexcrichton
Copy link
Member

@lilianmoraru I'd like to understand more about the whitelist of functions here that aren't tested, but other than that it should just be a rebase away from landing

@lilianmoraru
Copy link

lilianmoraru commented Aug 16, 2017

Note to self for follow-up(I've lost track of what needs to follow, I try to remember some of the points):

@pepyakin
Copy link

pepyakin commented Aug 16, 2017

@lilianmoraru

I might be terribly wrong, but isn't binaryen builds from the master?

https://github.com/rust-lang/rust/blob/master/src/ci/docker/scripts/emscripten-wasm.sh#L31

curl "https://storage.googleapis.com/wasm-llvm/builds/linux/lkgr.json" \
  | jq '.repositories.binaryen.hash'

at time of this writing, points on 21d06ae witch seems to be master, meaning that mentioned fixes there already.

had to force-push..

It doesn't mean that you did something wrong.

@alexcrichton
Copy link
Member

Thanks for taking this on @lilianmoraru! Other than the .travis.yml weirdness, was there anything I could help out with?

@lilianmoraru
Copy link

lilianmoraru commented Aug 16, 2017

@alexcrichton Nah. I pushed my changes to @malbarbo's branch( malbarbo#1 ), now I'll just wait.
I do worry that a rebase is not enough because I saw some new code that should probably be under the cfg(target_os = "emscripten"), cfg(not(target_os = "emscripten")).

alexcrichton added a commit to alexcrichton/libc that referenced this pull request Aug 27, 2017
Rebase of rust-lang#610 and also move emscripten up much higher in the hierarchy to
ensure that it doesn't have too much of a ripple effect on other platforms.

This involved moving down a good number of definitions, but hopefully was done
with care to not break anything!
alexcrichton added a commit to alexcrichton/libc that referenced this pull request Aug 27, 2017
Rebase of rust-lang#610 and also move emscripten up much higher in the hierarchy to
ensure that it doesn't have too much of a ripple effect on other platforms.

This involved moving down a good number of definitions, but hopefully was done
with care to not break anything!
bors added a commit that referenced this pull request Aug 27, 2017
Add asmjs/wasm32 to CI

Rebase of #610 and also move emscripten up much higher in the hierarchy to
ensure that it doesn't have too much of a ripple effect on other platforms.

This involved moving down a good number of definitions, but hopefully was done
with care to not break anything!
@alexcrichton
Copy link
Member

I've rebased this in #742 with what I had in mind, basically just copying around a lot more things to avoid many changes to libc-test and ensure everything is validated on emscripten.

@alexcrichton
Copy link
Member

Thanks again though for the patch @malbarbo!

alexcrichton added a commit to alexcrichton/libc that referenced this pull request Aug 27, 2017
Rebase of rust-lang#610 and also move emscripten up much higher in the hierarchy to
ensure that it doesn't have too much of a ripple effect on other platforms.

This involved moving down a good number of definitions, but hopefully was done
with care to not break anything!
bors added a commit that referenced this pull request Aug 27, 2017
Add asmjs/wasm32 to CI

Rebase of #610 and also move emscripten up much higher in the hierarchy to
ensure that it doesn't have too much of a ripple effect on other platforms.

This involved moving down a good number of definitions, but hopefully was done
with care to not break anything!
@lilianmoraru
Copy link

Personally, I would of liked to preserve the history of @malbarbo's work :(

@alexcrichton
Copy link
Member

Egad sorry! I've merged it all into the other PR

bors added a commit that referenced this pull request Aug 27, 2017
Add asmjs/wasm32 to CI

Rebase of #610 and also move emscripten up much higher in the hierarchy to
ensure that it doesn't have too much of a ripple effect on other platforms.

This involved moving down a good number of definitions, but hopefully was done
with care to not break anything!
bors added a commit that referenced this pull request Aug 27, 2017
Add asmjs/wasm32 to CI

Rebase of #610 and also move emscripten up much higher in the hierarchy to
ensure that it doesn't have too much of a ripple effect on other platforms.

This involved moving down a good number of definitions, but hopefully was done
with care to not break anything!
bors added a commit that referenced this pull request Aug 27, 2017
Add asmjs/wasm32 to CI

Rebase of #610 and also move emscripten up much higher in the hierarchy to
ensure that it doesn't have too much of a ripple effect on other platforms.

This involved moving down a good number of definitions, but hopefully was done
with care to not break anything!
@malbarbo
Copy link
Contributor Author

Thanks @alexcrichton for finishing this! I was worried that removing the functions white list would be too disturbing. Moving emscripten up in the hierarchy was the right choice.

@lilianmoraru sorry for not replaying to you messages..., I've been very busy lately. And thank you for helping moving this forward.

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