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

Refuse to codegen an upstream static. #100211

Merged
merged 1 commit into from
Aug 15, 2022
Merged

Refuse to codegen an upstream static. #100211

merged 1 commit into from
Aug 15, 2022

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Aug 6, 2022

Fixes #85401

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 6, 2022
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(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 Aug 6, 2022
@michaelwoerister
Copy link
Member

Looks good. thanks for the PR, @cjgillot!
@bors r+

@bors
Copy link
Contributor

bors commented Aug 9, 2022

📌 Commit b114b2d2dce25bf47f0a186d825971d2366bc880 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 Aug 9, 2022
@tmiasko
Copy link
Contributor

tmiasko commented Aug 9, 2022

Using metadata only crates for code generation is unsupported. Implementing such a support would need larger design work. See #38913 for the previous discussion.

This effectively removes a safeguard against situation where a static item is being code generated in non-local crate, which indicates either a bug or an unsupported build configuration. Instead it erroneously duplicates those static items in each crate that references them.

@michaelwoerister
Copy link
Member

@bors r-

Those sound like valid concerns. Thanks, @tmiasko!

Is there another way to address this issue?

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 9, 2022
@cjgillot
Copy link
Contributor Author

cjgillot commented Aug 9, 2022

This second version limits itself to preventing the ICE, relying on rustc_codegen_ssa to emit an error during linkage.

@cjgillot cjgillot 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 Aug 9, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@michaelwoerister
Copy link
Member

Yeah, that seems uncontroversial to me. Thanks for following up.
I'm not sure if we still can say that this fixes issue #85401 though 🙂
I think it's unclear if we even want to support the exact example from #85401.

You can r=me, @cjgillot.

I'll let you decide if you want to close #85401 via this PR or not.

@cjgillot cjgillot changed the title Account for mir_for_ctfe in foreign is_mir_available. Refuse to codegen an upstream static. Aug 10, 2022
@cjgillot
Copy link
Contributor Author

About closing the issue: this PR removes the ICE and explains why this does not compile. I'll consider it as fixed.
@bors r=michaelwoerister

@bors
Copy link
Contributor

bors commented Aug 10, 2022

📌 Commit d3fee8d 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 Aug 10, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 15, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 15, 2022
@bors
Auto merge of rust-lang#100569 - matthiaskrgr:rollup-9450lzs, r=matth…
9b4ea39 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#100211 (Refuse to codegen an upstream static.)
 - rust-lang#100277 (Simplify format_args builtin macro implementation.)
 - rust-lang#100483 (Point to generic or arg if it's the self type of unsatisfied projection predicate)
 - rust-lang#100506 (change `InlineAsmCtxt` to not talk about `FnCtxt`)
 - rust-lang#100534 (Make code slightly more uniform)
 - rust-lang#100566 (Use `create_snapshot_for_diagnostic` instead of `clone` for `Parser`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d6b6503 into rust-lang:master Aug 15, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 15, 2022
@cjgillot cjgillot deleted the ctfe-mir-available branch August 16, 2022 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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.65.0
Development

Successfully merging this pull request may close these issues.

-Zalways-encode-mir=yes misses MIR for statics
7 participants
@cjgillot @rust-highfive @michaelwoerister @bors @tmiasko @rust-log-analyzer @rustbot