Review : More groundwork

[lqd]

More groundwork on:

• enums. The enum test now compiles and works.
• c-enums
• updated binaryen to get br_table support and the latest fixes
• with br_table, match expressions on variants were implemented (MIR switch, no switchInt yet). An example showcasing those 3 features was added as well.
• first pass at tuple assignments
• a slightly simplified cmp.rs from std/core compiles (without derives right now, because they require more type support than just i32s, and panic, etc)
• with enums, a simple version of Option can be compiled, so there's an example of that.
• a plan for the calling convention (which I'll have to check with @eddyb), especially for functions returning structs: temporarily hacked with copies to make it work first for review, until a more efficient/correct method is implemented
• modified some examples to have paths that match what #[derive] expects, and added Copy/Clone derivation to the struct example
• 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 (eg switchInt)
• fixed a couple latent bugs, related to binaryen's Duff's device, and monomorphizations recorded "as seen" (which I'll also have to check with @eddyb) to not trans again
• brought back the entry_fn to handle non #[main] main fns for example, as to not require the use of attributes

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)

[brson]

r? @eholk do you mind looking at this? You've been better at scrutinizing the actual code in these PR's than I.

[brson]

This is so amazing. r=me if eholk is happy.

[lqd]

@brson even after @eholk reviews the other parts, I'd still like to work on cleaning up the calling convention before merging, but I didn't want to delay review of all the other parts any longer :)

[eholk]

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.

[lqd]

@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.

[lqd]

Here goes, crossing my fingers I don't mess up the history as I always do.

[lqd]

@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 :)

[kripken]

since wasm doesn't have isize yet IIRC

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.)

[lqd]

@kripken yeah that's what I figured as well, thank you for confirming 😄

[eholk]

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 :)

[brson]

Thanks @eholk ! \o/

[lqd]

cool, thank you @eholk. I'll make another PR addressing the comments in this review soon. This'll be without a doubt my last PR for a while as I'll be leaving on vacation 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 :smile: