Skip to content
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

Fix alloc_jemalloc debug feature #43648

Merged
merged 2 commits into from
Aug 29, 2017
Merged

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Aug 4, 2017

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.

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Aug 8, 2017

📌 Commit 1027f8f has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Aug 8, 2017

⌛ Testing commit 1027f8fff824b3bf057c31c1e1469677d6bdb260 with merge f08220c0e95737b0533cd4dfae968671c2b95ec3...

@bors
Copy link
Contributor

bors commented Aug 8, 2017

💔 Test failed - status-travis

@arielb1
Copy link
Contributor

arielb1 commented Aug 8, 2017

On x86_64-gnu-debug:

Compiling rand v0.0.0 (file:///checkout/src/librand)
[00:24:12] <jemalloc>: /checkout/src/liballoc_jemalloc/../jemalloc/src/jemalloc.c:2520: Failed assertion: "usize == isalloc(ptr, config_prof)"
[00:24:13] error: Could not compile `rand`.

@arielb1 arielb1 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 8, 2017
@arielb1
Copy link
Contributor

arielb1 commented Aug 8, 2017

Looks bad. Someone needs to investigate. Can you reproduce this locally @RalfJung ?

@carols10cents
Copy link
Member

Ping @RalfJung -- looks like there was a problem, are you able to reproduce it locally?

@RalfJung
Copy link
Member Author

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 Cargo.toml and build.rs? Are there any circumstances under which this can even work?

@RalfJung
Copy link
Member Author

RalfJung commented Aug 16, 2017

Just a plain ./x.py build works fine here. How can I tell which additional settings this "debug" CI build sets? It seems to pass --enable-debug to... something, and that's where the trail ends. There is no debug option in config.toml either.

EDIT: Somehow, --debug seems to set CFG_ENABLED_DEBUG inside the configure script. No idea how that happens, but I can then see how various debug options get enabled there. Now re-building with these options set.

@alexcrichton
Copy link
Member

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 x86_64-gnu-debug builder is configured a little differently than just ./x.py build, and you can find the relevant arguments in the relevant Dockerfile

@RalfJung
Copy link
Member Author

what's happening (I think) is that we've hit a bug in jemalloc (or our usage of it) which causes a runtime assertion.

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.

@RalfJung
Copy link
Member Author

@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?

@RalfJung
Copy link
Member Author

I locally reproduced the error and confirmed that after rebasing on top of #43911, the error is gone.

@arthurprs
Copy link
Contributor

Good news :)

@RalfJung
Copy link
Member Author

With jemalloc 4.5 having landed, I rebased this one. Locally, things work now, let's see what the CI says.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Aug 20, 2017

📌 Commit 47f1095 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Aug 20, 2017

⌛ Testing commit 47f1095c15d631241df64ca164885dd391636062 with merge 53cc684696e9a3a36f3c6dea4df078e0b46222b8...

@bors
Copy link
Contributor

bors commented Aug 20, 2017

💔 Test failed - status-travis

@RalfJung
Copy link
Member Author

RalfJung commented Aug 20, 2017

[00:50:12] <jemalloc>: /checkout/src/liballoc_jemalloc/../jemalloc/src/jemalloc.c:2654: Failed assertion: "usize == isalloc(tsd_tsdn(tsd), ptr, config_prof)"

[00:50:12] error: Could not compile `libc`.

[00:50:12] warning: build failed, waiting for other jobs to finish...

[00:50:24] error: build failed

Strange. I did test a local build, and I thought I had debug-jemalloc enabled.
(EDIT: Ah, I don't think I built all the stages.)

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.

@RalfJung
Copy link
Member Author

I can locally reproduce the error when doing ./x.py build (rather than just building a stage 1 compiler). So this is not exactly the same error as before the upgrade -- it's the same message, but it doesn't appear in stage 1 any more. Or so.

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 alexcrichton assigned alexcrichton and unassigned brson Aug 22, 2017
@carols10cents
Copy link
Member

@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?

@alexcrichton
Copy link
Member

@RalfJung mind opening an issue and otherwise I we'll need to close this?

@alexcrichton
Copy link
Member

@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.
@RalfJung
Copy link
Member Author

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.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Aug 29, 2017

📌 Commit 16fc74c has been approved by alexcrichton

@RalfJung
Copy link
Member Author

RalfJung commented Aug 29, 2017

Should this get rollup priority? Seems like a waste of a full CI run... (Cc @frewsxcv)

@bors
Copy link
Contributor

bors commented Aug 29, 2017

⌛ Testing commit 16fc74c with merge 630e02f...

bors added a commit that referenced this pull request Aug 29, 2017
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.
@arielb1 arielb1 added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 29, 2017
@arielb1
Copy link
Contributor

arielb1 commented Aug 29, 2017

@RalfJung

Not sure. This is entirely the sort of thing that causes odd failures on odd platforms.

@bors
Copy link
Contributor

bors commented Aug 29, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 630e02f to master...

@bors bors merged commit 16fc74c into rust-lang:master Aug 29, 2017
@RalfJung RalfJung deleted the jemalloc-debug branch July 10, 2018 09:05
bors added a commit that referenced this pull request Aug 2, 2018
enable jemalloc assertions when configured to do so

This is essentially a re-submission of the functional part of #43648. I was unable to reproduce the issue I had back then, maybe something changed somewhere to no longer trigger the assertion.

Fixes #44152
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants