[1.30 beta] Test suite of the jemalloc-ctl crate is failing #54478
Comments
pietroalbini
added
I-nominated
regression-from-stable-to-beta
|
Those errors seem to imply that global allocator registration may have broken? |
|
It seems like it may specifically be a problem with rustdoc tests - a normal library test passes as expected. |
|
Bisection over nightly builds indicates that bug was injected between nightly-2018-08-06-x86_64-unknown-linux-gnu (73c7873) and nightly-2018-08-14-x86_64-unknown-linux-gnu (d5a448b). In the details block are the bors commits from that range: % git log --author bors 73c7873..d5a448b --format=oneline
|
|
Okay I bisected this and identified the bug as being injected by #52993 |
|
Adding @alexcrichton to the assignee list in case they have some special insight into how best to address it. (But, as I noted on #52993, I am hoping we can get away with just undoing one small part of that PR, rather than backing out the whole thing, which seems like it might cause more trouble than it would resolve) |
pnkfelix
added
A-allocators
|
(also, tagging with both T-compiler and T-libs for now, though either team is free to untag the other if they want to take responsibility for resolving this.) |
|
Thanks for the cc @pnkfelix! The issue here can be reduced to the following: use std::alloc::*;
#[global_allocator]
static ALLOC: A = A;
static mut HIT: bool = false;
struct A;
unsafe impl GlobalAlloc for A {
unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
HIT = true;
System.alloc(layout)
}
unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
System.dealloc(ptr, layout);
}
}
fn main() {
assert!(unsafe { HIT });
}compiled and run as... So specifically the bug here comes with the The test actually has always failed on Windows (where the symbols don't trump one another). The regression here is Linux (and probably OSX?) specific. I think the "real fix" here is to not compile tests dynamically but compile them the same way as basically 99% of the compiles in Rust, statically. |
|
Er sorry, I misspoke! Turns out I saw the regression on Linux and not on Windows because during process startup we allocate memory but don't allocate memory on Windows (wow!). If you amend the program to do Digging more into this it's still, however, a bug that the compiler ever accepted this code. For example consider: use std::alloc::*;
use std::process::Command;
use std::env;
#[global_allocator]
static ALLOC: A = A;
static mut HITS: usize = 0;
struct A;
unsafe impl GlobalAlloc for A {
unsafe fn alloc(&self, layout: Layout) -> *mut u8 {
HITS += 1;
System.alloc(layout)
}
unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) {
System.dealloc(ptr, layout);
}
}
fn main() {
unsafe {
let start = HITS;
Box::new(2); // our own crate makes an allocation
assert_eq!(HITS, start + 1);
drop(env::var("X")); // force libstd itself to make an allocation
assert!(HITS > start + 1);
}
}This test will pass with and without What's happening here is that on stable Rust the executable, with On Windows, however, symbols can't shadow one another so you've actually got two allocators here. The executable has its own allocator as does the standard library, which can very quickly lead to segfaults as memory is passed between them (if they're not both wired up to All in all tl;dr; the compiler should have rejected this test previously due to rustdoc compiling tests dynamically. We should enact two changes from this discovery:
The first one is... "just sort of somewhere in the compiler add a check" and the second one is right here. |
|
@alexcrichton hmm... I'm curious whether either of the changes you describe should be targetted toward the beta channel (because this is a beta regression). I suspect the change to the compiler to detect two allocators linked into one program is not a high priority item for beta. The change to rustdoc ... seems like it should go into the beta (in order to address regressions like the one flagged here on the jemalloc-ctl crate) ... but I worry that its such a severe change that it is more risky to put into beta than to just let the regressions happen to jemalloc-ctl and other crates doing similar things in their rustdoc.... I suppose the best thing would be to do a crater run dedicated to just the rustdoc change and see what comes of that. |
|
Also, talk about an ancient bit of code: Lines 99 to 106 in 6c9c045 |
We need to be fast to do that though, full Crater run now take up to 5 days. |
|
@pietroalbini let me see if I can get a PR up |
|
Filed as PR #54939. I Put it up now because I don't know if my local test run will finish before I hit the sack. Also, that PR does not yet have a unit test for the regression. But it should be enough to throw into crater, I hope |
…c-tests, r=<try> [WIP] rustdoc: don't prefer dynamic linking in doc tests This is an attempt to address the regression in #54478 This may be a case where the cure is worse than the disease, at least in the short term... cc @alexcrichton
|
@pnkfelix right yeah changing the compiler to have new errors shouldn't be backported, and I would also be very wary of backporting changes to (sorry I was waiting for a local build to see the viability of a PR but kept forgetting to go back to it) |
pnkfelix
added
the
T-rustdoc
pnkfelix
removed
T-compiler
…c-tests, r=QuietMisdreavus rustdoc: don't prefer dynamic linking in doc tests This is an attempt to address the regression in #54478 This may be a case where the cure is worse than the disease, at least in the short term... cc @alexcrichton
|
Fixed and backported, should work in the next beta release. |
|
I hate to ask, but: @pietroalbini , I think we still need a regression test, right? |
|
Ugh, there was no regression test in that PR. |
pnkfelix
added
the
E-needs-test
|
(But the lack of a regression test does not need to block the release, of course) |
|
Manually checked the issue is actually fixed, just to be sure. |
…t-jemalloc-ctl, r=nikomatsakis Regression test for rust-lang#54478. This is a regression test for rust-lang#54478. I confirmed that it fails on: rustdoc 1.30.0-beta.12 (96a2298 2018-10-04) and passes on: rustdoc 1.31.0-nightly (f99911a 2018-10-23) Fix rust-lang#54478
…t-jemalloc-ctl, r=nikomatsakis Regression test for rust-lang#54478. This is a regression test for rust-lang#54478. I confirmed that it fails on: rustdoc 1.30.0-beta.12 (96a2298 2018-10-04) and passes on: rustdoc 1.31.0-nightly (f99911a 2018-10-23) Fix rust-lang#54478
Rollup of 22 pull requests Successful merges: - #53507 (Add doc for impl From for Waker) - #53931 (Gradually expanding libstd's keyword documentation) - #54965 (update tcp stream documentation) - #54977 (Accept `Option<Box<$t:ty>>` in macro argument) - #55138 (in which unused-parens suggestions heed what the user actually wrote) - #55173 (Suggest appropriate syntax on missing lifetime specifier in return type) - #55200 (Documents `From` implementations for `Stdio`) - #55245 (submodules: update clippy from 5afdf8b to b1d0343) - #55247 (Clarified code example in char primitive doc) - #55251 (Fix a typo in the documentation of RangeInclusive) - #55253 (only issue "variant of the expected type" suggestion for enums) - #55254 (Correct trailing ellipsis in name_from_pat) - #55269 (fix typos in various places) - #55282 (Remove redundant clone) - #55285 (Do some copy editing on the release notes) - #55291 (Update stdsimd submodule) - #55296 (Set RUST_BACKTRACE=0 for rustdoc-ui/failed-doctest-output.rs) - #55306 (Regression test for #54478.) - #55328 (Fix doc for new copysign functions) - #55340 (Operands no longer appear in places) - #55345 (Remove is_null) - #55348 (Update RELEASES.md after destabilization of non_modrs_mods) Failed merges: r? @ghost
This is not a spurious failure (the test output is consistent between runs), but we need to investigate why this is happening.
0.2.0regressed from stable to beta (build log) cc @sfacklerThe text was updated successfully, but these errors were encountered: