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.

Sign up for GitHub

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

Jump to bottom

rustllvm: Use .init_array rather than .ctors #71234

Merged
merged 1 commit into from May 9, 2020
Merged

rustllvm: Use .init_array rather than .ctors #71234

merged 1 commit into from May 9, 2020

Conversation

maurer
Copy link
Contributor

@maurer maurer commented Apr 17, 2020

LLVM TargetMachines default to using the (now-legacy) .ctors
representation of init functions. Mixing .ctors and .init_array
representations can cause issues when linking with lld.

This happens in practice for:

  • Our profiling runtime which is currently implicitly built with
    .init_array since it is built by clang, which sets this field.
  • External C/C++ code that may be linked into the same process.

Fixes: #71233

@rust-highfive
Copy link
Collaborator

r? @cuviper

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

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

cuviper commented Apr 17, 2020

We've been talking about compatibility on #71233 -- let's get a real CI build that I can actually test on an old x86_64-unknown-linux-gnu system, at least.

@bors try

You also mentioned the possibility of a Target option for this. I think we probably do want that, as well as a -Z option for manual control, similar to -Zplt, -Zrelro-level, etc. We can default to .init_array if compatibility is broad enough, but still have a way to fall back, just like Clang 10 is still offering -fno-use-init-array.

@bors
Copy link
Contributor

bors commented Apr 17, 2020

⌛ Trying commit 1d84563f375a5977ffa8eb640aa80a84959b4bf1 with merge b87bd037d8b09eee9115142adb1d61112cb4865c...

@bors
Copy link
Contributor

bors commented Apr 17, 2020

⌛ Trying commit 1d84563f375a5977ffa8eb640aa80a84959b4bf1 with merge ab42f00561856008c3af29d224b26c802e4563d2...

@cuviper
Copy link
Member

cuviper commented Apr 17, 2020

Sorry about that -- I didn't see bors responding, and now I got it twice... oh well.

@Dylan-DPC-zz
Copy link

@bors try

@bors
Copy link
Contributor

bors commented Apr 17, 2020

⌛ Trying commit 1d84563f375a5977ffa8eb640aa80a84959b4bf1 with merge 04ec1be6a7a65dd0304d2b35993ae7daf7abbc13...

@maurer
Copy link
Contributor Author

maurer commented Apr 17, 2020

I added in -Z use-ctors and a TargetOptions field to govern .ctors use as requested, defaulting to .init_array usage.

@petrochenkov petrochenkov self-assigned this Apr 18, 2020
petrochenkov
petrochenkov reviewed Apr 18, 2020
View reviewed changes
src/librustc_session/options.rs Outdated Show resolved Hide resolved
src/librustc_codegen_llvm/back/write.rs Outdated Show resolved Hide resolved
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 18, 2020
@maurer
Copy link
Contributor Author

maurer commented Apr 19, 2020

I've made the requested changes.

@petrochenkov petrochenkov removed their assignment Apr 19, 2020
@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 19, 2020
@bors
Copy link
Contributor

bors commented Apr 22, 2020

☔ The latest upstream changes (presumably #71374) made this pull request unmergeable. Please resolve the merge conflicts.

@nnethercote
Copy link
Contributor

When you rebase over #71374 please make sure you preserve the new alphabetical order. Also, it looks like you haven't updated the tests, please see the new comment at the top of DebuggingOptions.

@maurer
Copy link
Contributor Author

maurer commented Apr 29, 2020

I've rebased the patch and added the test entry nnethercote asked for.

@maurer
Use .init_array rather than .ctors
0e7d5be 
LLVM TargetMachines default to using the (now-legacy) .ctors
representation of init functions. Mixing .ctors and .init_array
representations can cause issues when linking with lld.

This happens in practice for:

* Our profiling runtime which is currently implicitly built with
  .init_array since it is built by clang, which sets this field.
* External C/C++ code that may be linked into the same process.

To support legacy systems which may use .ctors, targets may now specify
that they use .ctors via the use_ctors attribute which defaults to
false.

For debugging and manual control, -Z use-ctors-section=yes/no will allow
manual override.

Fixes: rust-lang#71233
@cuviper
Copy link
Member

cuviper commented May 5, 2020

The patch looks OK, but I'd like a second opinion on making this change -- maybe @joshtriplett?

@joshtriplett
Copy link
Member

This seems reasonable to me; we should certainly use .init_array. However, lld really does need to handle the mix of .ctors and .init_array; that's not a Rust-specific issue.

@cuviper
Copy link
Member

cuviper commented May 9, 2020

@bors r+

@bors
Copy link
Contributor

bors commented May 9, 2020

📌 Commit 0e7d5be has been approved by cuviper

@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 May 9, 2020
@RalfJung RalfJung mentioned this pull request May 9, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 9, 2020
@bors
Auto merge of rust-lang#72041 - RalfJung:rollup-xivrvy2, r=RalfJung
bad3bf6 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#69406 (upgrade chalk and use chalk-solve/chalk-ir/chalk-rust-ir)
 - rust-lang#71185 (Move tests from `test/run-fail` to UI)
 - rust-lang#71234 (rustllvm: Use .init_array rather than .ctors)
 - rust-lang#71508 (Simplify the `tcx.alloc_map` API)
 - rust-lang#71555 (Remove ast::{Ident, Name} reexports.)

Failed merges:

r? @ghost
@bors bors merged commit ce05553 into rust-lang:master May 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cuviper cuviper cuviper left review comments

@petrochenkov petrochenkov petrochenkov approved these changes

Assignees

@cuviper cuviper

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Coverage is not generated when using lld as linker
8 participants
@maurer @rust-highfive @cuviper @bors @Dylan-DPC-zz @nnethercote @joshtriplett @petrochenkov