Upgrade to nightly

[joshuawarner32]

NOTE: Do not merge yet! Shenanigans abound!

... but on the surface it seems to work.

[joshuawarner32]

FYI, I was just playing around with this (awesome stuff!). I thought this might be helpful. If not, don't hesitate to close this. No worries.

[brson]

Thanks @joshuawarner32 !

@eholk do you want to take a peek at this before I blindly merge?

[eholk]

I'd like to take a look. I should be able to get to it this afternoon. I can go ahead and complete the merge when I'm done if that's alright with you.

On Tue, Sep 13, 2016 at 2:59 PM Brian Anderson notifications@github.com wrote:

Thanks @joshuawarner32 https://github.com/joshuawarner32 !

@eholk https://github.com/eholk do you want to take a peek at this before I blindly merge?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/brson/mir2wasm/pull/39#issuecomment-246839258, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGdJoedqZgn29lDOlvab-FkPOs4fzURks5qpxzegaJpZM4J5mzG .

[joshuawarner32]

@brson Also be aware that several of the caveats mentioned above still apply; I haven't gotten a chance to look at the handling of Asserts and CheckedBinaryOperations. Also, the only testing I've done is to run cargo run -q -- rust-examples/nocore-hello-world.rs.

Also, I just saw https://github.com/rust-lang/rust/pull/36339 (llvm-based wasm) - does that mean mir2wasm is not the "right" direction to go?

[kripken]

@joshuawarner32

does that mean mir2wasm is not the "right" direction to go?

Both approaches (mir2wasm and llvm-based) are worthwhile in my opinion. Each has different things that makes it useful:

• mir2wasm should be able to beat all other options - including native llvm-based builds - on compile time, easily.
• the llvm-based approach can more easily integrate with emscripten libraries like GL, SDL, etc.
• mir2wasm is more under rust's control - it doesn't need to use existing emscripten "ABI" type things like how it manages the stack, etc. That means it's harder to integrate with existing libraries, but allows doing things in more rust-specific ways. In particular for simple "pure computation" uses of rust on the web, mir2wasm could emit more focused and compact code.

[eholk]

I'd like to see cargo test pass before we merge this. I just gave it a try and after updating compiletest_rs, I got the following error (among others):

test [run-compile-pass] tests/compile-pass/struct-derive.rs ... FAILED with exit code Some(101)
stdout: 

stderr: 
 error: internal compiler error: ../src/librustc/ty/subst.rs:457: Type parameter `Self/#0` (Self/0) out of range when substituting (root type=Some(&Self)) substs=[]

thread 'main' panicked at 'Box<Any>', ../src/librustc_errors/lib.rs:590
stack backtrace:
   1:     0x7f711db2b173 - std::sys::backtrace::tracing::imp::write::h4b09e6e8c01db097
   2:     0x7f711db3bd1d - std::panicking::default_hook::{{closure}}::h1d3243f546573ff4
   3:     0x7f711db3a3aa - std::panicking::default_hook::h96c288d728df3ebf
   4:     0x7f711db3a9a8 - std::panicking::rust_panic_with_hook::hb1322e5f2588b4db
   5:     0x7f7122110328 - std::panicking::begin_panic::hb7c71bbc491f561c
   6:     0x7f71223b5935 - rustc::session::opt_span_bug_fmt::{{closure}}::h6ad7caa20ac20c4b
   7:     0x7f712230ec85 - rustc::session::opt_span_bug_fmt::h38eb39636c07401c
   8:     0x7f712230eb14 - rustc::session::span_bug_fmt::heb3fdb6acac8d0c7
   9:     0x7f712236c2de - <rustc::ty::subst::SubstFolder<'a, 'gcx, 'tcx> as rustc::ty::fold::TypeFolder<'gcx, 'tcx>>::fold_ty::h17c67c094233ff36
  10:     0x7f712236bf5a - <rustc::ty::subst::SubstFolder<'a, 'gcx, 'tcx> as rustc::ty::fold::TypeFolder<'gcx, 'tcx>>::fold_ty::h17c67c094233ff36
  11:     0x7f7122ca29af - rustc::ty::structural_impls::<impl rustc::ty::fold::TypeFoldable<'tcx> for &'tcx rustc::ty::TyS<'tcx>>::fold_with::h36c63211b891560a
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/librustc/ty/structural_impls.rs:508
  12:     0x7f7122cb0c05 - <T as rustc::ty::subst::Subst<'tcx>>::subst_spanned::h1e2909ff760b6f53
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/librustc/ty/subst.rs:352
  13:     0x7f7122ca4c7d - rustc::ty::subst::Subst::subst::h273373dc5ac91f64
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/librustc/ty/subst.rs:331
  14:     0x7f7122cfd7cf - mir2wasm::monomorphize::apply_ty_substs::hef3b5edb9ee9e940
                        at /usr/local/google/home/eholk/wasm/mir2wasm/src/monomorphize.rs:20
  15:     0x7f7122cfa812 - mir2wasm::trans::BinaryenFnCtxt::type_layout_with_substs::hcddbf3b0d36a58a0
                        at /usr/local/google/home/eholk/wasm/mir2wasm/src/trans.rs:1080
  16:     0x7f7122cfa7b3 - mir2wasm::trans::BinaryenFnCtxt::type_layout::h599f742f9b96d87d
                        at /usr/local/google/home/eholk/wasm/mir2wasm/src/trans.rs:1074
  17:     0x7f7122cf2a01 - mir2wasm::trans::BinaryenFnCtxt::trans_assignment::hc4ecc00d5d562645
                        at /usr/local/google/home/eholk/wasm/mir2wasm/src/trans.rs:634
  18:     0x7f7122cec046 - mir2wasm::trans::BinaryenFnCtxt::trans::hfde2fbe142e22c3a
                        at /usr/local/google/home/eholk/wasm/mir2wasm/src/trans.rs:278
  19:     0x7f7122ceaaa2 - <mir2wasm::trans::BinaryenModuleCtxt<'v, 'tcx> as rustc::hir::intravisit::Visitor<'v>>::visit_fn::h51bafc35e583b5f8
                        at /usr/local/google/home/eholk/wasm/mir2wasm/src/trans.rs:184
  20:     0x7f7122ca7701 - rustc::hir::intravisit::walk_trait_item::hb1ec63911b017ada
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/librustc/hir/intravisit.rs:662
  21:     0x7f7122ca8aac - rustc::hir::intravisit::Visitor::visit_trait_item::hab873576703f59aa
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/librustc/hir/intravisit.rs:145
  22:     0x7f7122cac5ff - rustc::hir::intravisit::walk_item::he9e3385587e0fc9d
                        at /usr/local/google/home/eholk/wasm/mir2wasm/<syntax macros>:2
  23:     0x7f7122ca873c - rustc::hir::intravisit::Visitor::visit_item::h1dc71540c0e4b486
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/librustc/hir/intravisit.rs:92
  24:     0x7f7122cac8e9 - rustc::hir::Crate::visit_all_items::hb6fdac0d00839bd9
                        at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/obj/../src/librustc/hir/mod.rs:415
  25:     0x7f7122cea005 - mir2wasm::trans::trans_crate::hb5f4874b54b7cb66
                        at /usr/local/google/home/eholk/wasm/mir2wasm/src/trans.rs:79
  26:     0x7f7122c4d239 - <mir2wasm::WasmCompilerCalls as rustc_driver::CompilerCalls<'a>>::build_controller::{{closure}}::h47f88d76f5b28e7e
                        at /usr/local/google/home/eholk/wasm/mir2wasm/src/bin/mir2wasm.rs:49
  27:     0x7f712276f8d7 - rustc_driver::driver::compile_input::{{closure}}::hdf708031c67974d4
  28:     0x7f7122760d55 - rustc_driver::driver::phase_3_run_analysis_passes::{{closure}}::hcac3a2328cd9259a
  29:     0x7f712272c695 - rustc_driver::driver::phase_3_run_analysis_passes::h9865057b323cad0e
  30:     0x7f712271d793 - rustc_driver::driver::compile_input::hc0edbed7edb3eb18
  31:     0x7f7122749626 - rustc_driver::run_compiler::h22d678d32fb7c300
  32:     0x7f7122c4c791 - mir2wasm::main::h79208956a14bce2e
                        at /usr/local/google/home/eholk/wasm/mir2wasm/src/bin/mir2wasm.rs:137
  33:     0x7f711db49046 - __rust_maybe_catch_panic
  34:     0x7f711db39961 - std::rt::lang_start::haaae1186de9de8cb
  35:     0x7f7122c4d353 - main
  36:     0x7f711cf60f44 - __libc_start_main
  37:     0x7f7122c17d48 - <unknown>
  38:                0x0 - <unknown>

I'm guessing @brson or someone else on the Rust team knows how to fix this pretty readily.

Incidentally, I had a similar in progress branch with the same problem. Just from a cursory look, @joshuawarner32's version seems better overall, since it seems to actually handle a few cases that I just aggressively deleted.

It'd also be good to update the readme with the specific Rust nightly that this builds with so that we have a known good revision the next time nightly breaks us.

[brson]

@joshuawarner32 Thanks for the caveats. Since this is such an early prototype still I'm happy to endure some breakage, as long as it looks good to @eholk and/or @lqd. And as @kripken mentioned, there are reasons to pursue both approaches, so even once the LLVM-based approach is merged I intend to keep plugging away at this one.

Maybe @eddyb can see what's being generated incorrectly to cause that. Otherwise I can poke at it later.

[eholk]

I guess a totally legitimate way to make the tests pass in this case is the xfail the failing ones. @brson's point about enduring breakage on an early prototype is a good one; it's probably better to track nightly a little closer than we're doing than have production-quality tests.

[joshuawarner32]

@eholk: Not sure if this helps, but based on some println debugging, it looks like the internal compiler errors are happening while processing the methods of the ne method of the pub trait PartialEq<Rhs: ?Sized = Self> traits in the test files.

For example, here's some of the relevant values as they appear at trans.rs:1083, just before the ICE (appogies for the horrible formatting; you can see the line number at the very end of that third line):

ty: &Self
self.did: DefId { krate: 0, node: DefIndex(90) => PartialEq::ne }
self.mir: Mir { basic_blocks: [BasicBlockData { statements: [StorageLive(var0), var0 = arg0, StorageLive(var1), var1 = arg1, StorageLive(tmp0), StorageLive(tmp1), tmp1 = &(*var0), StorageLive(tmp2), tmp2 = &(*var1)], terminator: Some(Terminator { source_info: SourceInfo { span: tests/compile-pass/operators.rs:103:42: 103:56, scope: scope1 }, kind: tmp0 = <Self as PartialEq<Rhs>>::eq(tmp1, tmp2) -> bb1 }), is_cleanup: false }, BasicBlockData { statements: [return = Not(tmp0), StorageDead(tmp0), StorageDead(tmp2), StorageDead(tmp1), StorageDead(var1), StorageDead(var0)], terminator: Some(Terminator { source_info: SourceInfo { span: tests/compile-pass/operators.rs:103:5: 103:58, scope: scope1 }, kind: return }), is_cleanup: false }], visibility_scopes: [VisibilityScopeData { span: tests/compile-pass/operators.rs:103:5: 103:58, parent_scope: None }, VisibilityScopeData { span: tests/compile-pass/operators.rs:103:39: 103:58, parent_scope: Some(scope0) }], promoted: [], return_ty: bool, var_decls: [VarDecl { mutability: Not, name: self(25), ty: &Self, source_info: SourceInfo { span: tests/compile-pass/operators.rs:103:11: 103:16, scope: scope1 } }, VarDecl { mutability: Not, name: other(99), ty: &Rhs, source_info: SourceInfo { span: tests/compile-pass/operators.rs:103:18: 103:23, scope: scope1 } }], arg_decls: [ArgDecl { ty: &Self, spread: false, debug_name: self(25) }, ArgDecl { ty: &Rhs, spread: false, debug_name: other(99) }], temp_decls: [TempDecl { ty: bool }, TempDecl { ty: &Self }, TempDecl { ty: &Rhs }], upvar_decls: [], span: tests/compile-pass/operators.rs:103:5: 103:58, cache: Cache { predecessors: RefCell { value: None } } }

[eddyb]

@joshuawarner32 Did my suggestion not fix it?

[lqd]

Sorry I haven't had the time to check this out (and am unfortunately unlikely to be able to do so in the immediate future) however there were a couple of things I thought I needed to mention :smile:

1) our generics/traits/fulfillment/monomorphization story is far from complete. It works on the simple test cases we have, but more complex cases would fail to compile and ICE with similar errors as the ones we're seeing here. 2) as commented on top of traits.rs, the whole thing is basically from miri (but apparently they've since updated it a lot). Specifically, from here https://github.com/solson/miri/blob/master/src/interpreter/terminator.rs — which we know works at least for their test cases and the most recent nightly they updated to (which seems recent). If this PR is updating to a similar nightly, maybe it'd be best to try their approach instead of trying to modify ours to match the more recent rustc API anyway ?

[joshuawarner32]

@eddyb - I didn't notice a difference in the outcome of the tests after I implemented your def_id to trait_id suggestion. But it does make sense - at least to someone with only a glancing familiarity in this code.

@lqd - Do you have a concrete suggestion on how to pull that in? It's not immediately obvious to me how to adapt that to the current interface. The only parts that look remotely equatable is the ImplMethod type. I experimentally pulled those changes in, with no differences.

Tiny bit of incremental progress on the internal compiler error. The following snippet seems to be the minimal code needed to trigger the error. Removing the body on the method defined in the trait (as opposed to the impl) is enough to no longer see the error.

pub trait Test {
    fn test(&self) { } // removing the body here gets us passed the ICE
}

impl Test for isize {
    fn test(&self) { }
}

Does that ring a bell for anyone?

[eddyb]

@joshuawarner32 Sounds like it uses the default body even when the method is implemented - but it's not using the trait substs, but the impl ones, with the trait default method.

[lqd]

@joshuawarner32 yes:

• traits::get_impl_method is miri's get_impl_method https://github.com/solson/miri/blob/master/src/interpreter/terminator.rs#L588
• traits::fulfill_obligation is miri's fulfill_obligation slightly modified to remove the EvalContext self parameter by passing the TyCtxt directly https://github.com/solson/miri/blob/master/src/interpreter/terminator.rs#L470
• traits::resolve_trait_method is miri's trait_method slightly modified in the same manner https://github.com/solson/miri/blob/master/src/interpreter/terminator.rs#L493

(There were also a couple of different error handling suggestions from eddy which miri didn't implement at the time)

It's very possible that this doesn't fix the issue indeed, but at least you won't have to update the current traits module to the newer rustc API.

It's not a problem if it doesn't fix it either, if @eholk is happy with the PR, I'd say let's merge it to get the benefits of the newer nightly, and then deal with this — as we'll have to fix other monomorphizations cases anyway!

[eholk]

Hmm, does binaryen still print out the result of running a program? It looks like some of our tests aren't showing the expected output. For example,

cargo run -- tests/compile-pass/nocore-hello-world.rs --run

doesn't give me any output, but the test is expected to print (i32.const 6).

[joshuawarner32]

@eholk: Perhaps you're referring to tests/run-pass/nocore-hello-world.rs, which in this PR currently prints (i32.const 0)?

I tracked that down to a problem with how I assumed checked ops would work in mir. How they actually work is the CheckedAdd operation actually gives something like a (i32, i32) for (value, overflow_bit) respectively - and then there's an assert operation to check the value of the overflow bit. Since I directly emitted a value instead of a struct/pair, it got emitted as a SetLocal, whereas the code to use it got emitted as a GetLocal followed by a Load to grab the particular field. Yeah. I'll fix that.

<side-note>Longer term, it's probably best to model (non-escaping) structs on the stack as exploded locals. Otherwise, the optimizer is going to have one hell of a time cleaning that up, since the WebAssembly memory model doesn't allow eliminating that store. But I bet that could easily become a rat's nest.</side-note>

[joshuawarner32]

Quick update:

I fixed a few of the outstanding problems. (In particular, @eholk, tests/run-pass/nocore-hello-world.rs now prints (i32.const 6) as it should.)

I'm still going to have a go at ICE failures, and possibly look at pulling in up-to-date miri code as @lqd suggested.

[eholk]

I just tried xfailing the tests that were failing, and unfortunately I ran into another issue. It looks like there's a deps test that runs afterwards and that is segfaulting now, at least for me. It seems to be trying to call a destructor on an std::vector that has already been freed.

Here's the stack:

* thread #1: tid = 23158, 0x00007ffff34af606 libstd-411f48d3.so`free + 150, name = 'mir2wasm-ce627d', stop reason = signal SIGSEGV: invalid address (fault address: 0x8)
  * frame #0: 0x00007ffff34af606 libstd-411f48d3.so`free + 150
    frame #1: 0x00007ffff60b7133 librustc_llvm-411f48d3.so`operator delete(void*) + 31
    frame #2: 0x0000555555583942 mir2wasm-ce627d33d91d8d02`__gnu_cxx::new_allocator<char>::deallocate(this=0x00007ffff0cbf120, __p="", (null)=140737233142491) + 32 at new_allocator.h:110
    frame #3: 0x000055555557ad6a mir2wasm-ce627d33d91d8d02`std::_Vector_base<char, std::allocator<char> >::_M_deallocate(this=0x00007ffff0cbf120, __p="", __n=140737233142491) + 50 at stl_vector.h:174
    frame #4: 0x00005555555746e1 mir2wasm-ce627d33d91d8d02`std::_Vector_base<char, std::allocator<char> >::~_Vector_base(this=0x00007ffff0cbf120) + 61 at stl_vector.h:160
    frame #5: 0x000055555556e407 mir2wasm-ce627d33d91d8d02`std::vector<char, std::allocator<char> >::~vector(this=0x00007ffff0cbf120) + 65 at stl_vector.h:416
    frame #6: 0x000055555562d9d4 mir2wasm-ce627d33d91d8d02`cashew::IStringSet::~IStringSet(this=0x00007ffff0cbf0f0) + 28 at istring.h:151
    frame #7: 0x0000555555631d40 mir2wasm-ce627d33d91d8d02`cashew::OperatorClass::~OperatorClass(this=0x00007ffff0cbf0f0) + 24 at parser.h:131
    frame #8: 0x0000555555631d5e mir2wasm-ce627d33d91d8d02`void __gnu_cxx::new_allocator<cashew::OperatorClass>::destroy<cashew::OperatorClass>(this=0x00005555558bb530, __p=0x00007ffff0cbf0f0) + 28 at new_allocator.h:124
    frame #9: 0x0000555555631036 mir2wasm-ce627d33d91d8d02`std::enable_if<std::allocator_traits<std::allocator<cashew::OperatorClass> >::__destroy_helper<cashew::OperatorClass>::value, void>::type std::allocator_traits<std::allocator<cashew::OperatorClass> >::_S_destroy<cashew::OperatorClass>(__a=0x00005555558bb530, __p=0x00007ffff0cbf0f0) + 35 at alloc_traits.h:281
    frame #10: 0x00005555556301e9 mir2wasm-ce627d33d91d8d02`void std::allocator_traits<std::allocator<cashew::OperatorClass> >::destroy<cashew::OperatorClass>(__a=0x00005555558bb530, __p=0x00007ffff0cbf0f0) + 35 at alloc_traits.h:405
    frame #11: 0x000055555562ed2c mir2wasm-ce627d33d91d8d02`void std::vector<cashew::OperatorClass, std::allocator<cashew::OperatorClass> >::_M_emplace_back_aux<char const (this=0x00005555558bb530, (null)=<no value available>, (null)=0x00007fffffffda80, (null)=0x00007fffffffda90) [8], bool, cashew::OperatorClass::Type>(char const (&) [8], bool&&, cashew::OperatorClass::Type&&) + 516 at vector.tcc:422
    frame #12: 0x000055555562e0a6 mir2wasm-ce627d33d91d8d02`void std::vector<cashew::OperatorClass, std::allocator<cashew::OperatorClass> >::emplace_back<char const (this=0x00005555558bb530, (null)=<no value available>, (null)=0x00007fffffffda80, (null)=0x00007fffffffda90) [8], bool, cashew::OperatorClass::Type>(char const (&) [8], bool&&, cashew::OperatorClass::Type&&) + 204 at vector.tcc:101
    frame #13: 0x000055555562da8e mir2wasm-ce627d33d91d8d02`cashew::Init::Init(this=0x00005555558bb568) + 106 at parser.cpp:125
    frame #14: 0x0000555555634325 mir2wasm-ce627d33d91d8d02`::__static_initialization_and_destruction_0(__initialize_p=1, __priority=65535) + 2557 at parser.cpp:148
    frame #15: 0x00005555556343c2 mir2wasm-ce627d33d91d8d02`::_GLOBAL__sub_I_parser.cpp() + 19 at parser.cpp:161
    frame #16: 0x00005555556356ed mir2wasm-ce627d33d91d8d02`__libc_csu_init + 77
    frame #17: 0x00007ffff28c5ed5 libc.so.6`__libc_start_main(main=(mir2wasm-ce627d33d91d8d02`main), argc=1, argv=0x00007fffffffdc08, init=(mir2wasm-ce627d33d91d8d02`__libc_csu_init), fini=<unavailable>, rtld_fini=<unavailable>, stack_end=0x00007fffffffdbf8) + 133 at libc-start.c:246
    frame #18: 0x000055555555fef9 mir2wasm-ce627d33d91d8d02`_start + 41

I'm tempted just to go ahead and open a bug to track this new issue and merge this PR, because I'd like to get it merged.

[joshuawarner32]

@eholk - then let me take a stab this evening at isolating the portions of the tests that are actually causing the ICE, and mark just those parts as xfail. I'd rather have mostly-working tests than mostly-broken tests.

[eholk]

@joshuawarner32 - sounds good! I like mostly working tests too :) As long as you're okay to keep working on them, I'm happy to wait. I just don't want you to feel like I'm putting up arbitrary roadblocks for you or anything like that.

[lqd]

an std::vector & libstdc++ ? is this related to binaryen ?

[joshuawarner32]

@eholk - I modified / xfailed the appropriate tests, and now everything seems to be passing.

Also, I'm not able to reproduce the std::vector / double-free issue you're seeing. I'm running macOS sierra, if that's helpful. Are you seeing that with just a cargo test?

[eholk]

Sorry @joshuawarner32, I didn't notice your update to this a few days ago. I tried it on my mac and also didn't see the crash. In the interest of moving things forward, I'm going to accept this and open an issue about the crash I'm seeing on Linux.

@lqd - I have some suspicions that it might be related to binaryen, but I don't have any real evidence to base this on yet.

[lqd]

Thanks @joshuawarner32! and thanks @eholk of course as well!