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

Place TLS initializers with relocations in .tdata #70720

Merged
merged 2 commits into from
Apr 3, 2020

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Apr 2, 2020

Should fix #70673, although I'm not sure how to test this. Perhaps @joshlf could find a MCVE?

Also adds more context to the FIXME.

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 2, 2020
@joshlf
Copy link
Contributor

joshlf commented Apr 2, 2020

I'll try to pull this and build and test locally. If you're developing on a Mac, you could also just try building alloc-tls as described in the original issue, and see if it both fails before this change and succeeds with this change.

@joshlf
Copy link
Contributor

joshlf commented Apr 3, 2020

OK, I built your patch locally, and with it, cargo test succeeds in building and running alloc-tls. That's not an MCVE, but hopefully that gives a bit more confidence that this is correct.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 3, 2020

I'd assume

#![feature(thread_local)]

#[thread_local]
static A: &u8 = &42;

fn main() {
    dbg!(*A);
}

would be a minimal repro, unsure how to test this without a mac though.

The PR looks fine to me, so

@bors r+

@bors
Copy link
Contributor

bors commented Apr 3, 2020

📌 Commit fffbcc8 has been approved by oli-obk

@bors bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2020
@joshlf
Copy link
Contributor

joshlf commented Apr 3, 2020

@oli-obk Right you are! Tested your proposed repro locally, and it both crashes the current compiler and also doesn't crash with this PR applied.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 3, 2020

Superb! Thanks for checking

@ecstatic-morse if you get around to it before this PR goes to bors, please add the repro to the test suite, if not let's keep the issue open as needs-test

@ecstatic-morse
Copy link
Contributor Author

Added the regression test.

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Apr 3, 2020

📌 Commit f030635 has been approved by oli-obk

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 3, 2020
Rollup of 9 pull requests

Successful merges:

 - rust-lang#69860 (Use associated numeric consts in documentation)
 - rust-lang#70576 (Update the description of the ticket to point at RFC 1721)
 - rust-lang#70597 (Fix double-free and undefined behaviour in libstd::syn::unix::Thread::new)
 - rust-lang#70640 (Hide `task_context` when lowering body)
 - rust-lang#70641 (Remove duplicated code in trait selection)
 - rust-lang#70707 (Remove unused graphviz emitter)
 - rust-lang#70720 (Place TLS initializers with relocations in .tdata)
 - rust-lang#70735 (Clean up E0502 explanation)
 - rust-lang#70741 (Add test for rust-lang#59023)

Failed merges:

r? @ghost
@bors bors merged commit 80690b0 into rust-lang:master Apr 3, 2020
@ecstatic-morse ecstatic-morse deleted the issue-70637 branch October 6, 2020 01:42
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.

ICE compiling alloc-tls
5 participants