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

Add -Z external-clangrt #121207

Merged
merged 3 commits into from
Mar 15, 2024
Merged

Add -Z external-clangrt #121207

merged 3 commits into from
Mar 15, 2024

Conversation

chriswailes
Copy link
Contributor

This adds the unstable -Z external-clangrt flag that will prevent rustc from emitting linker paths for the in-tree LLVM sanitizer runtime library.

@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2024

r? @michaelwoerister

rustbot has assigned @michaelwoerister.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 16, 2024
@petrochenkov
Copy link
Contributor

This looks similar to one of unimplemented parts of -C link-self-contained (+/-sanitizers).

@chriswailes
Copy link
Contributor Author

I certainly don't want to duplicate functionality. If there is a way to do the same thing with a different combination of flags we'd prefer that.

@michaelwoerister
Copy link
Member

Here is some documentation on the unstable values for -C link-self-contained:
https://doc.rust-lang.org/unstable-book/compiler-flags/codegen-options.html#link-self-contained

That indeed seems like the preferable way of exposing this knob. (Thanks @petrochenkov, I would have missed that!)

@chriswailes, do you want to look into updating the PR accordingly?

@chriswailes
Copy link
Contributor Author

@michaelwoerister Yes, I'll give this a look and see if it covers our use case.

@chriswailes chriswailes mentioned this pull request Feb 22, 2024
Closed
@michaelwoerister
Copy link
Member

During the discussion of the alternative PR #121420 it turned out that expressing this via -C link-self-contained is not such a good fit after all.

Let's add the -Z flag. Thank you for your patience, @chriswailes!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 13, 2024

📌 Commit f1d594e has been approved by michaelwoerister

It is now in the queue for this repository.

@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 Mar 13, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 13, 2024
@matthiaskrgr
Rollup merge of rust-lang#121207 - chriswailes:z-external-clangrt, r=…
1891141 
…michaelwoerister

Add `-Z external-clangrt`

This adds the unstable `-Z external-clangrt` flag that will prevent rustc from emitting linker paths for the in-tree LLVM sanitizer runtime library.
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 13, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 13, 2024
@bors
Auto merge of rust-lang#122444 - matthiaskrgr:rollup-jf146j9, r=matth…
2dae8fa 
…iaskrgr

Rollup of 12 pull requests

Successful merges:

 - rust-lang#104353 (Add CStr::bytes iterator)
 - rust-lang#120699 (Document `TRACK_DIAGNOSTIC` calls.)
 - rust-lang#121207 (Add `-Z external-clangrt`)
 - rust-lang#122397 (Various cleanups around the const eval query providers)
 - rust-lang#122416 (Various style improvements to `rustc_lint::levels`)
 - rust-lang#122422 (compiletest: Allow `only-unix` in test headers)
 - rust-lang#122424 (fix: typos)
 - rust-lang#122425 (Increase timeout for new bors bot)
 - rust-lang#122426 (Fix StableMIR `WrappingRange::is_full` computation)
 - rust-lang#122430 (Generate link to `Local` in `hir::Let` documentation)
 - rust-lang#122434 (pattern analysis: rename a few types)
 - rust-lang#122437 (pattern analysis: remove `MaybeInfiniteInt::JustAfterMax`)

r? `@ghost`
`@rustbot` modify labels: rollup
@chriswailes
Add -Z external-sanitizer-runtime
2a9d1ed 
This adds the unstable `-Z external-sanitizer-runtime` flag that will
prevent rustc from emitting linker paths for the in-tree LLVM sanitizer
runtime library.
@chriswailes
Split a complex conditional into separate statements
bf2858a
@chriswailes
Copy link
Contributor Author

I've added the commit that breaks apart the conditional as well as rebased the patch on ToT.

@klensy
Copy link
Contributor

klensy commented Mar 13, 2024

Need r+ again after push

michaelwoerister
michaelwoerister reviewed Mar 13, 2024
View reviewed changes
Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the code, @chriswailes. I think it's not quite right now though.

compiler/rustc_codegen_ssa/src/back/link.rs
// both executables and dynamic shared objects. On most other platforms the
// runtimes are currently distributed as static libraries which should be
// linked to executables only.
if matches!(crate_type, CrateType::Rlib | CrateType::Staticlib) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this code path can even ever be hit for those crate types since those don't involve a linking step. In any case, the comment about seems fit better for the next condition. Also, is there a reason why Windows is not mentioned anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rushed the rebase process and incorrectly resurrected the old version of the comments. Fixed in the new commit.

compiler/rustc_codegen_ssa/src/back/link.rs

if matches!(crate_type, CrateType::Dylib | CrateType::Cdylib | CrateType::ProcMacro)
&& (sess.target.is_like_osx || sess.target.is_like_msvc)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, isn't this the wrong way around now? That is, if this is some kind of dylib, then we do need to link the runtime on osx- and msvc-like platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Fixed.

@michaelwoerister
Copy link
Member

Thanks, @chriswailes!
@bors r+

@bors
Copy link
Contributor

bors commented Mar 15, 2024

📌 Commit f46acea has been approved by michaelwoerister

It is now in the queue for this repository.

@matthiaskrgr matthiaskrgr mentioned this pull request Mar 15, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2024
@bors
Auto merge of rust-lang#122534 - matthiaskrgr:rollup-p95i3ph, r=matth…
d7723b2 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#121207 (Add `-Z external-clangrt`)
 - rust-lang#122174 (diagnostics: suggest `Clone` bounds when noop `clone()`)
 - rust-lang#122471 (preserve span when evaluating mir::ConstOperand)
 - rust-lang#122515 (Pass the correct DefId when suggesting writing the aliased Self type out)
 - rust-lang#122523 (Ensure RPITITs are created before def-id freezing)
 - rust-lang#122526 (Docs for `thir::ExprKind::Use` and `thir::ExprKind::Let`)
 - rust-lang#122527 (Clean up AstConv)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2b8fc6f into rust-lang:master Mar 15, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2024
@rust-timer
Unrolled build for rust-lang#121207
c28317f 
Rollup merge of rust-lang#121207 - chriswailes:z-external-clangrt, r=michaelwoerister

Add `-Z external-clangrt`

This adds the unstable `-Z external-clangrt` flag that will prevent rustc from emitting linker paths for the in-tree LLVM sanitizer runtime library.
@ObsidianMinor ObsidianMinor mentioned this pull request Mar 29, 2024
Closed
@rcvalle rcvalle mentioned this pull request Aug 28, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelwoerister michaelwoerister michaelwoerister left review comments

Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.78.0
Development

Successfully merging this pull request may close these issues.
6 participants
@chriswailes @rustbot @petrochenkov @michaelwoerister @bors @klensy