Skip to content

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
Contributor

r? @brson

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

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Aug 8, 2017

📌 Commit 1027f8f has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Aug 8, 2017

⌛ Testing commit 1027f8fff824b3bf057c31c1e1469677d6bdb260 with merge f08220c0e95737b0533cd4dfae968671c2b95ec3...

@bors
Copy link
Collaborator

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
Collaborator

bors commented Aug 20, 2017

📌 Commit 47f1095 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Aug 20, 2017

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

@bors
Copy link
Collaborator

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
Collaborator

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
Collaborator

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
Collaborator

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