Panic during unit tests (coerce_match) #20055
Comments
|
I should also note that the test code pub fn main() {
let _: Box<[int]> = if true { box [1i, 2, 3] } else { box [1i] };
let _: Box<[int]> = match true { true => box [1i, 2, 3], false => box [1i] };
// Check we don't get over-keen at propagating coercions in the case of casts.
let x = if true { 42 } else { 42u8 } as u16;
let x = match true { true => 42, false => 42u8 } as u16;
} |
|
@jroesch is also having this issue. He says that tests stopped passing for him sometime after the night of December 16th |
|
Here's the backtrace from a crash log: LLDB gives the following stack trace: frame #0 points at a macro to generate red-black tree functions, though as you can see |
|
I'm going to attempt a bisect, though it'll take a while. |
|
According to the bisect, the bad commit is 46eb724. |
|
Which was introduced in #19769 /cc @nick29581 |
|
As it turns out, #19769 is what introduced the |
nrc
added
the
I-crash
|
I was running This is running with the merge of #20083., commit d10642e. |
|
i also see this on my mac |
|
When this is fixed, can we make sure coerce_match.rs moves to run-pass-valgrind |
|
I am now also getting a second failure: |
|
@jroesch yeah I added that second test when I was investigating the first one (when I was working on stuff related to |
|
@pnkfelix thanks for the update. They are just a minor annoyance when running the test suite. |
|
Here is a reduced test case for input: I have compared the generated llvm-ir with and without the Without with the |
|
Here is a result of further investigation: I do not know how valid it is to try to plug in different llvm-passes in an attempt to expose a code-generation bug, but it seemed to produce interesting information (in terms of certain combinations seeming to fall victim to the same bug). Both of these example commands are being fed the reduced test case from the previous comment (named |
|
(One can emulate the same effect on a generated .bc file via the llvm |
|
Ah, an interesting new detail: in the below revision to the running example, passing Example with workaround: use std::boxed::Box;
#[cfg(not(workaround))]
pub fn main() {
let _: Box<[i8]> = match true {
true => Box::new([1i8, 2, 3]),
false => Box::new([1i8]),
};
}
#[cfg(workaround)]
pub fn main() {
let _: Box<[i8]> = match true {
true => Box::new([1i8, 2, 3]),
false => { Box::new([1i8]) }
};
} |
|
I looked into this some more. In particular, I made a Its possible I made a mistake along the way, but there are some pretty funky things being illustrated in this test. https://gist.github.com/pnkfelix/6dc4fb620c8742100598
(Update: there was indeed a mistake in my port: I forgot to null terminate my strings, and so the first printf call was printing both strings, which presumably just happened to be laid out next to each other in the generated data segment. Nonetheless, the above gist is still useful, in that it continues to illustrate the bug injected by using |
|
Another gist I have developed seems to show a case where optimizations are disabled and yet we still have a spurious https://gist.github.com/pnkfelix/567213f949925ce33697 (Note that main difference between this gist and the one from the previous comment is that I introduced an extra block around the first match arm: |
|
Yay, I found a different example, a generalization of the original one, where one can observe the erroneous frees occurring even when optimizations are disabled. https://gist.github.com/pnkfelix/2e2b73092c2fd5b38228 (This is very similar to my previous gists; the main difference is that I have added four separate match arms, each building a differently sized The errors you get with and without |
|
And here's a variant of that same example; again, this causes an error both with and without optimizations on Mac OS X: // #![crate_type="lib"]
pub fn foo(box_1: fn () -> Box<[i8; 1]>,
box_2: fn () -> Box<[i8; 2]>,
box_3: fn () -> Box<[i8; 3]>,
box_4: fn () -> Box<[i8; 4]>) {
println!("Hello World 1");
let _: Box<[i8]> = match 3 {
1 => box_1(),
2 => box_2(),
3 => box_3(),
_ => box_4(),
};
println!("Hello World 2");
}
pub fn main() {
fn box_1() -> Box<[i8; 1]> { Box::new( [1i8] ) }
fn box_2() -> Box<[i8; 2]> { Box::new( [1i8, 2] ) }
fn box_3() -> Box<[i8; 3]> { Box::new( [1i8, 2, 3] ) }
fn box_4() -> Box<[i8; 4]> { Box::new( [1i8, 2, 3, 4] ) }
foo(box_1, box_2, box_3, box_4);
}(The above gist has the advantage that it uses |
|
(The program from the previous comment also causes a core-dump in a Linux VM; Ubuntu 64-bit 14.10...) |
…s original L-/R-value state. This fixes a subtle issue where temporaries were being allocated (but not necessarily initialized) to the (parent) terminating scope of a match expression; in particular, the code to zero out the temporary emitted by `datum.store_to` is only attached to the particular match-arm for that temporary, but when going down other arms of the match expression, the temporary may falsely appear to have been initialized, depending on what the stack held at that location, and thus may have its destructor erroneously run at the end of the terminating scope. Test cases to appear in a follow-up commit. Fix rust-lang#20055
Note that I have not yet managed to expose any bug in `trans::expr::into_fat_ptr`; it would be good to try to do so (or show that the use of `.to_lvalue_datum` there is sound).
trans: When coercing to `Box<Trait>` or `Box<[T]>`, leave datum in it's original L-/R-value state. This fixes a subtle issue where temporaries were being allocated (but not necessarily initialized) to the (parent) terminating scope of a match expression; in particular, the code to zero out the temporary emitted by `datum.store_to` is only attached to the particular match-arm for that temporary, but when going down other arms of the match expression, the temporary may falsely appear to have been initialized, depending on what the stack held at that location, and thus may have its destructor erroneously run at the end of the terminating scope. FIx #20055. (There may be a latent bug still remaining in `fn into_fat_ptr`, but I am so annoyed by the test/run-pass/coerce_match.rs failures that I want to land this now.)
|
Awesome work @pnkfelix ✨ |
Currently on bd90b93
Running on OSX
cc'ing @kballard and @barosl since I saw both of them also had this issue in #rust-internals on IRC
The text was updated successfully, but these errors were encountered: