-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[1.30 beta] Test suite of the collection crate is failing #54477
Comments
visited at T-compiler triage. Assigning to self to do initial investigation. |
I have locally reproduced bug (by cloning the
The bug also reproduces on the current nightly. (My previous claim was erroneous, as I forgot to update my nightly before running it.) Bisecting now. |
Bisected over nightlies to determine this was injected between 7061b27 and 02cb8f2
|
Here's the log of bors merges over the range between 7061b27 and 02cb8f2
|
Based solely on skimming that list of PR's, I would be inclined to double-check the its possible its actually the MIRI change... but that's not my first guess. In the meantime, I'm working on reducing the test case. I currently have it down to a single standalone file, but its still 3.6K lines... |
Okay, at this point I have confirmed that reverting #53564 fixes this bug. We should revert #53564. I haven't finished reducing my test case as much as I would like, but I can post what I have. Its in this gist: Here's a demo of stable working: And here's what happens on beta:
|
Unassigning myself as I cannot devote further time to this and it seems like it should be handled by the libs team at this point. The good idea that I just had, unfortunately too late:
that would be a good way to reduce the test case to something really trivial. |
Thanks so much for flagging this @pietroalbini and tracking this down @pnkfelix! That's an impressive investigation @pnkfelix! I can take it from here with reverts |
Interestingly, miri does not see any UB here. Not that this makes the bug less of a problem, but this could mean it's "just" a plain functional mistake (or UB that miri cannot detect ;) |
…ckler Fix a regression in 1.30 by reverting rust-lang#53564 Investigation on rust-lang#54477 revealed rust-lang#53564 as the culprit in the regression for that crate. I've reproduced the regression with the [detailed test cases provided](rust-lang#54477 (comment)). While we figure out how to fix the regression this commit reverts the current culprit.
Okay I got back from my evening at a waterpark and decided to try implementing my idea of an instruemented I've gotten as far as generating the following log from the instrumentation
|
Okay now I have a minimized regression test (play) use std::collections::VecDeque;
fn main() {
let mut vecdeque_13 = VecDeque::from(vec![ ]);
let mut vecdeque_29 = VecDeque::from(vec![ 0 ]);
vecdeque_29.insert(0, 30 );
vecdeque_29.insert(1, 31 );
vecdeque_29.insert(2, 32 );
vecdeque_29.insert(3, 33 );
vecdeque_29.insert(4, 34 );
vecdeque_29.insert(5, 35 );
println!("vecdeque_13: {:?}", vecdeque_13);
println!("vecdeque_29: {:?}", vecdeque_29);
println!("Invoking: `vecdeque_13.append(&mut vecdeque_29)`");
vecdeque_13.append(&mut vecdeque_29);
println!("vecdeque_13: {:?}", vecdeque_13);
assert_eq!(vecdeque_13, VecDeque::from(vec![30, 31, 32, 33, 34, 35, 0]));
} Unsurprisingly the issue is exposed by the |
(it probably wouldn't be a terrible idea to try to adapt the much larger test input into a unit test for |
Btw, your test case still fails if I set |
Fixed on beta. Closing this. |
Did a regression test land? |
Uh, I don't think so (looking at the PR). |
@pnkfelix I know I'm late to the party, but here's something that would have come in handy for testcase minimization: https://github.com/blt/bughunt-rust It attempts to implement automated discovery crashes and incorrect behavior on Rust stdlib data structures. It's very similar to QuickCheck, but uses an instrumentation-guided fuzzer instead of PRNG as source of input. It operates on command streams and supports minimizing them, so far only by minimizing the amount of commands. It already has a VecDeque test harness and obviously correct but not optimized implementation to cross-check std::VecDeque output against. |
I removed the original file that more completely captured the original crate's tests, as its source crate (https://crates.io/crates/collection) is licensed under GPL3, and I suspect that license is not loose enough for me to put into our repo under our MIT/Apache licensing. (Would it be an option to attach the GPL3 licesne to just the one test? Probably. But do I want to bother with it that that point? Nope!)
…ts, r=nikomatsakis Regression tests for issue rust-lang#54477. At some point someone may want to revisit PR rust-lang#53564 it would be really good to have regression tests for rust-lang#54477 before that happens. :)
Rollup of 13 pull requests Successful merges: - #55280 (Add libproc_macro to rust-src distribution) - #55469 (Regression tests for issue #54477.) - #55504 (Use vec![x; n] instead of iter::repeat(x).take(n).collect()) - #55522 (use String::from() instead of format!() macro to construct Strings.) - #55536 (Pass suggestions as impl Iterator instead of Vec) - #55542 (syntax: improve a few allocations) - #55558 (Tweak `MatcherPos::matches`) - #55561 (Fix double_check tests on big-endian targets) - #55573 (Make sure the `aws` executable is in $PATH on macOS) - #55574 (Use `SmallVec` within `MoveData`.) - #55575 (Fix invalid_const_promotion test on some archs) - #55578 (Made doc example of `impl Default for …` use `-> Self` instead of explicit self type) - #55582 (Remove unused import copy from publish_toolstate.py)
This is not a spurious failure (the test output is consistent between runs), but we need to investigate why this is happening.
0.1.1
regressed from stable to beta (build log) cc @krlThe text was updated successfully, but these errors were encountered: