Fix alloc_jemalloc debug feature#43648
Conversation
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
|
@bors: r+ |
|
📌 Commit 1027f8f has been approved by |
|
⌛ Testing commit 1027f8fff824b3bf057c31c1e1469677d6bdb260 with merge f08220c0e95737b0533cd4dfae968671c2b95ec3... |
|
💔 Test failed - status-travis |
|
On |
|
Looks bad. Someone needs to investigate. Can you reproduce this locally @RalfJung ? |
|
Ping @RalfJung -- looks like there was a problem, are you able to reproduce it locally? |
|
Sorry somehow I didn't see the emails notifying me about this PR. I will try to reproduce this. However, as I already mentioned, I don't really know what I am doing here. @alexcrichton you wrote the line I am patching here, but that was 2 years ago... do you remember if there was a reason that the feature does not have the same name in |
|
Just a plain EDIT: Somehow, |
|
Ah unfortunately I don't think I'll be of much help here :(. Everything here looks correct to me, and what's happening (I think) is that we've hit a bug in jemalloc (or our usage of it) which causes a runtime assertion. As I think you've discovered at this point the |
Yeah that was also what I thought. We haven't actually tested jemalloc-debug all the time due to the bug this PR is fixing. Now that we do, that test fails. I don't think I can fix that, this needs someone who knows things about jemalloc. |
|
@arthurprs any chance you could give updating jemalloc to 4.5 another try? Maybe that helps with this PR, and it's something we'd probably want anyway? |
|
I locally reproduced the error and confirmed that after rebasing on top of #43911, the error is gone. |
|
Good news :) |
1027f8f to
47f1095
Compare
|
With jemalloc 4.5 having landed, I rebased this one. Locally, things work now, let's see what the CI says. |
|
@bors: r+ |
|
📌 Commit 47f1095 has been approved by |
|
⌛ Testing commit 47f1095c15d631241df64ca164885dd391636062 with merge 53cc684696e9a3a36f3c6dea4df078e0b46222b8... |
|
💔 Test failed - status-travis |
Strange. I did test a local build, and I thought I had debug-jemalloc enabled. It seems like we actually still hit a jemalloc assertion. This is not a bug introduced by this PR; it is a bug we had all along but failed to detect because jemalloc assertions were not actually enabled on the debug builder. |
|
I can locally reproduce the error when doing Anyway I have no idea what to do about this. I just wanted to fix a funny discrepancy in the name of the feature flag, not delve into horrible jemalloc internals.^^ I hope someone more knowledgable can take over from here... |
|
@alexcrichton what should be done with this PR? who might be best to take over to figure out why we're hitting a jemalloc assertion? |
|
@RalfJung mind opening an issue and otherwise I we'll need to close this? |
|
@bors: r- This got back in the queue? |
At least, I think that's how it should be. 'debug' is how the feature is called in Cargo.toml.
47f1095 to
16fc74c
Compare
|
I opened #44152. I also changed this PR to essentially a noop -- but I think that status is better than still having this wrong flag in build.rs without any comment. |
|
@bors: r+ |
|
📌 Commit 16fc74c has been approved by |
|
Should this get rollup priority? Seems like a waste of a full CI run... (Cc @frewsxcv) |
Fix alloc_jemalloc debug feature At least, I think that's how it should be. 'debug' is how the feature is called in liballoc_jemalloc/Cargo.toml and libstd/Cargo.toml. I verified this by making the build script panic rather than adding `--enable-debug`, and without this PR, the panic does not occur even when I set `debug-jemalloc = true` in config.toml. With the PR, the panic occurs as expected. However, I actually have no idea what I am doing here.
|
Not sure. This is entirely the sort of thing that causes odd failures on odd platforms. |
|
☀️ Test successful - status-appveyor, status-travis |
At least, I think that's how it should be. 'debug' is how the feature is called in liballoc_jemalloc/Cargo.toml and libstd/Cargo.toml. I verified this by making the build script panic rather than adding
--enable-debug, and without this PR, the panic does not occur even when I setdebug-jemalloc = truein config.toml. With the PR, the panic occurs as expected.However, I actually have no idea what I am doing here.