Review : More groundwork #27
Conversation
- enums. The enum test now compiles. - c-enums - some match support (switch, no switchint yet). An example showcasing those 3 features was added as well. - a slightly simplified cmp.rs from libstd compiles (without derives right now, because they require more type support than just i32s) - with enums, Option can be compiled, so there's an example of that. - first pass for the calling convention, and especially for functions returning structs: temporarily hacked with memcpys to make it work first, until a more efficient/correct method is implemented - modified some examples to have paths that match what #[derive] expects - with Option and some of the previous work, a simplified example of IntoIter and Iterator works - added successful compile-pass tests, mostly extracted from the current examples - added xfail tests to document two features needing to be done - fixed a couple latent bugs, related to binaryen's Duff's device and monomorphizations recorded "as seen" to not trans again - brought back the entry_fn to handle non #[main] main fns for example, to not require the use of attributes
|
r? @eholk do you mind looking at this? You've been better at scrutinizing the actual code in these PR's than I. |
|
This is so amazing. r=me if eholk is happy. |
|
Sorry, I was on vacation for the past few days. It'll be a little bit before I can review this, but I'll try to do it by the end of the week if that's alright. |
|
@eholk no problem really (the 14th's a bank holiday anyway), by then, I will have added (probably tomorrow) another couple commits to this PR to 1) add CLI parsing that will ease testing (and makes developing easier in general) as we talked about (but not yet assert! support as it seems Binaryen traps with a C++ exception, which I'm not sure can interact well from our side, but I'll investigate more) and 2) a first cleanup of the "memcpy" (using >1 bytes instead of just 1! because I only recently realized linear memory was little endian and offsets were in bytes... so that'll fix some of the weird behavior I was seeing in more complex cases than the ones I've been testing with so far). I definitely want to get rid of this copy soon (which will be transparent anyway, and doesn't impact the rest of your review) but I didn't have the time to do it yet. |
Quickly added -q, -O, --run, -h CLI options parsing, in order to make both manual testing and unit testing easier. Now we could have our own run-pass (but to make those tests useful, we would also require something like assert! and panic! which are not implemented yet)
|
Here goes, crossing my fingers I don't mess up the history as I always do. |
|
@eholk here you go, those are the modifications I mentioned last time (it's hard to detect/debug wasm memory errors until it can be exported to a js uint8array or maybe even just inspected through the interpreter). The commits also include some additional cleanup, and fixing compiling operators.rs isize on x64 (maybe we should have automated tests on multiple os/arch in this repo as well eventually) — which made me wonder, since wasm doesn't have isize yet IIRC, I thought the best way for now to handle them was to force them to i32s at compile time until we handle more types, what do you think ? Until you can review this, I'll probably look at assert! and panic! a bit more. On osx, binaryen trapping's behavior could be of use (ie something about the error is printed on the console, and the process doesn't return 0), while on windows it showed a MS failure UI and didn't display any information about the error, so we'll see (maybe it behaves differently there in Debug vs Release who knows). Being able to catch a C++ exception through a C binding through Rust FFI seems a bit improbable :) |
- udpate binaryen to get the tracing debugging API - add tracing to the options (not hooked up to the CLI) - optimize a bit the poor man's memcpy by copying 1-8 bytes at a time - fix offsets in struct handling - slight cleanup + compiling on x64
Yes, current wasm is "wasm32", where the pointer size is 32-bit. So you can basically treat this like a 32-bit target where isize is 32-bit. (In the future a wasm64 is planned, however, it's long-term and also will probably be rare in practice.) |
|
@kripken yeah that's what I figured as well, thank you for confirming 😄 |
| wasm::print_i32((x.ge(&y)) as i32); | ||
| } | ||
|
|
||
| pub mod marker { |
There was a problem hiding this comment.
Are these traits all copied from libcore/libstd, or are these reimplemented from scratch?
There was a problem hiding this comment.
as mentioned in the PR it's a slightly simplified cmp.rs from std/core :)
I just removed the derives (which come with their own requirements, eg assert_receiver_is_clone etc, which we may not end up wanting to port, but mostly because the generated impls for some of those traits required u64s and such :) and also I commented most of the cmp.rs impl macros for most if not all of the types other than i32. Quite a modification, but apart from that it's the regular rust traits.
The iterator.rs example however has a reimplementation of IntoInter and Iterator (since the iter support pulls a lot of other stuff which don't work yet)
| @@ -844,13 +1195,75 @@ fn rust_ty_to_binaryen<'tcx>(t: Ty<'tcx>) -> BinaryenType { | |||
| fn sanitize_symbol(s: &str) -> String { | |||
| s.chars().map(|c| { | |||
| match c { | |||
| '<' | '>' | ' ' => '_', | |||
| '<' | '>' | ' ' | '(' | ')' => '_', | |||
There was a problem hiding this comment.
This can be in a followup PR, but we probably want this to make different characters to different strings. As it stands now, something like foo<bar> and foo(bar) both map to foo_bar_, which is also a name someone can type. This could potentially lead to weird bugs caused by name collisions.
I suspect using a reasonably common C++ name mangling scheme might be the right approach here.
|
Sorry about taking so long to finish looking over this. I'm really impressed with how many features this adds, and how quickly you've been able to implement so me! r=me / lgtm / +1 / whatever the right way for us to indicate approval :) |
|
Thanks @eholk ! \o/ |
|
cool, thank you @eholk. I'll make another PR addressing the comments in this review soon. By the time I get back, the work upgrading LLVM in Rust and Emscripten to get wasm support "for free" will be finished so that's exciting 😄 |
More groundwork on:
Marking the PR for review as I'm not sure we would want it to land as is, but it's been a week's worth of work already and I didn't want to delay the review of the other parts any longer. (eg I think we'd want to have a cleaner/correct calling convention implementation to land)