Upgrade to nightly #39
Conversation
| @@ -534,6 +532,16 @@ impl<'v, 'tcx: 'v> BinaryenFnCtxt<'v, 'tcx> { | |||
| TerminatorKind::Return => { | |||
| /* handled during bb creation */ | |||
| } | |||
| TerminatorKind::Assert { ref target, .. } => { | |||
| // TODO: An assert is not a GOTO!!! | |||
There was a problem hiding this comment.
I wasn't sure what to do with this.
There was a problem hiding this comment.
|
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. |
| @@ -282,6 +277,9 @@ impl<'v, 'tcx: 'v> BinaryenFnCtxt<'v, 'tcx> { | |||
| StatementKind::Assign(ref lvalue, ref rvalue) => { | |||
| self.trans_assignment(lvalue, rvalue, &mut binaryen_stmts); | |||
| } | |||
| StatementKind::StorageLive(ref var) => {} | |||
| StatementKind::StorageDead(ref var) => {} | |||
There was a problem hiding this comment.
You can use _ instead of ref var to eliminate warnings about unused variables.
| } | ||
| } | ||
| } | ||
| let mut is_fn_panic = Some(self.did) == self.tcx.lang_items.panic_fn(); |
There was a problem hiding this comment.
This shouldn't be mut. In fact, it's only used once, in the condition below. If you switch the "then" and "else" bodies, you can use this comparison as the condition.
Similar story with is_entry_fn which could be Some(nid) == self.node_id below.
|
Thanks @joshuawarner32 ! @eholk do you want to take a peek at this before I blindly merge? |
|
I'd like to take a look. I should be able to get to it this afternoon. I On Tue, Sep 13, 2016 at 2:59 PM Brian Anderson notifications@github.com
|
|
@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 Also, I just saw rust-lang/rust#36339 (llvm-based wasm) - 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:
|
|
I'd like to see 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. |
|
@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. |
|
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. |
| match fulfill_obligation(tcx, trait_ref) { | ||
| traits::VtableImpl(vtable_impl) => { | ||
| let impl_did = vtable_impl.impl_def_id; | ||
| let mname = tcx.item_name(def_id); | ||
| // Create a concatenated set of substitutions which includes those from the impl | ||
| // and those from the method: | ||
| let impl_substs = vtable_impl.substs.with_method_from(substs); | ||
| let substs = tcx.mk_substs(impl_substs); | ||
| let substs = substs.rebase_onto(*tcx, def_id, vtable_impl.substs); |
There was a problem hiding this comment.
I think def_id here might need to be trait_id.
|
@eholk: Not sure if this helps, but based on some println debugging, it looks like the 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): |
|
@joshuawarner32 Did my suggestion not fix it? |
|
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 😄
|
|
@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 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 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? |
|
@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. |
|
@joshuawarner32 yes:
(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! |
|
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, doesn't give me any output, but the test is expected to print |
|
@eholk: Perhaps you're referring to I tracked that down to a problem with how I assumed checked ops would work in mir. How they actually work is the <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> |
|
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 Here's the stack: 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. |
|
@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. |
|
@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. |
|
an std::vector & libstdc++ ? is this related to binaryen ? |
|
@eholk - I modified / xfailed the appropriate tests, and now everything seems to be passing. Also, I'm not able to reproduce the |
|
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. |
|
Thanks @joshuawarner32! and thanks @eholk of course as well! |
NOTE: Do not merge yet! Shenanigans abound!
... but on the surface it seems to work.