Closed eholk closed 8 years ago
Yeah this looks like one of the issues about substs/generics/monomorphization I mentioned before https://github.com/brson/mir2wasm/issues/17#issuecomment-236712595 (like this stack trace seems to show).
Substs need to be tracked for the current BinaryenFnCtxt
, at least so that type_size()
and type_layout()
can use the current substs (and not empty substs), and trans_assignment()
in this stack trace can pass the correct substs to get the layout like so let dest_layout = self.type_layout_with_substs(dest_ty, self.substs);
instead of https://github.com/brson/mir2wasm/blob/master/src/trans.rs#L648 and so on.
I had worked on this very thing (eg part of this commit https://github.com/lqd/mir2wasm/commit/837eda704cba422eeccdd55382df3b9dbe7d3cfa but it also built upon temporary previous work, a hacky branch to try and compile libcore, so the code is not really usable as is unfortunately). IIRC, this was absolutely a required thing to do, and fixed some problematic cases on the previous nightly though, and would certainly fix some on the new nightly as well. At least the tests catch the problem now.
Another problematic case showed up with code similar to https://github.com/lqd/mir2wasm/blob/e5f4f6b56cd9906dbae0fdd5d8c555250ff7e018/rust-examples/option.rs and seemed less related to substs per se, compared to monomorphization/resolving trait methods, and there also is the substs-related "possible duplicated fns" from another bullet point in https://github.com/brson/mir2wasm/issues/17#issuecomment-236712595
Thanks for the investigation!
I opened this so we could have a tracking bug but go ahead and merge #39. Unfortunately, as soon as I did that I found another issue, which I mentioned at https://github.com/brson/mir2wasm/pull/39#issuecomment-248446874
I think we should be good here
Awesome. Are the tests that were failing still xfailed? If so, we should enable them again.
Sounds good. I'll close the bug.
As far as xfail versus compile-fail, I think of xfail as "these are tests that should work, but we know they don't and we'll fix them later." The tests in compile-fail are ones that are intentionally invalid and therefore we want to be sure they actually produce an error if you try to compile them. Is the test that got moved to compile-fail in this category?
No, this test is intended to compile and run, and we should really xfail it and move it back to the compile-pass folder. Let's do this when we can
Sounds good. Just to be sure, this was cmp.rs
?
We should probably just remove the compile-fail
directory, since this is all behavior that should have already been tested by rustc
, unless we expected there to be things that by design do not work in mir2wasm even though they do in Rust.
yup, cmp.rs Yeah we can probably remove the directory. Some failures we'd expect to have in our tests would be probably run-fails eg panic! or assert! and not compile failures indeed
A lot of our tests are showing this error after our latest nightly bump. Below is a representative stack trace.