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

Check for intrinsics before coercing to a function pointer #84404

Merged
merged 1 commit into from
Apr 22, 2021

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Apr 21, 2021

Return an error if coercing function items / non-capturing closures
to a common function pointer type would require reifying an intrinsic.

Turns ICE reported in #84297 into a proper error.

Return an error if coercing function items / non-capturing closures
to a common function pointer type would require reifying an intrinsic.
@rust-highfive
Copy link
Collaborator

r? @estebank

(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 21, 2021
@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

@bors r

Seems OK; we may eventually want to support this with a shim generated in-place, but doesn't seem worth the hassle right now and should be a good change otherwise.

@bors
Copy link
Contributor

bors commented Apr 22, 2021

📌 Commit 75732dd has been approved by Mark-Simulacrum

@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 22, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 22, 2021
Rollup of 7 pull requests

Successful merges:

 - rust-lang#84343 (Remove `ScopeTree::closure_tree`)
 - rust-lang#84376 (Uses flex to fix formatting of h1 at any width)
 - rust-lang#84377 (Followup to rust-lang#83944)
 - rust-lang#84396 (Update LLVM submodule)
 - rust-lang#84402 (Move `sys_common::rwlock::StaticRWLock` etc. to `sys::unix::rwlock`)
 - rust-lang#84404 (Check for intrinsics before coercing to a function pointer)
 - rust-lang#84413 (Remove `sys::args::Args::inner_debug` and use `Debug` instead)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f180c1e into rust-lang:master Apr 22, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 22, 2021
@tmiasko tmiasko deleted the intrinsics-in-coercion-lub branch April 22, 2021 21:53
@pietroalbini pietroalbini added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 30, 2021
@pietroalbini
Copy link
Member

@rust-lang/compiler this PR fixes a beta regression, #84297. Sorry for the late ping, could the backport be approved before next week?

@Mark-Simulacrum
Copy link
Member

Hm, this is actually not the right fix, upon further examination; or needs additional work. https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=01fb7cb94c3eda2cfffca68b09b6a709 compiles on stable, and this PR introduces an error rather than ICE, but we need to accept this code...

@tmiasko
Copy link
Contributor Author

tmiasko commented Apr 30, 2021

I don't think this should be backported, I wrote more in #84297 (comment).

@Mark-Simulacrum
Copy link
Member

Likely a revert of #81238 would be sufficient for the time being, cc @RalfJung

@RalfJung
Copy link
Member

RalfJung commented May 1, 2021

I replied in #84297.

@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 3, 2021
@Mark-Simulacrum
Copy link
Member

Dropping beta-nom here as we're going to end up reverting a different PR instead

pnkfelix added a commit to pnkfelix/rust that referenced this pull request Jun 4, 2021
…pping`.

Test was added in PR rust-lang#84404.

The intent here is: The `copy`/`copy_overlapping` intrinsics are going through
some flip-flopping now of "are they intrinsics or not". We can achieve the same
effect that the test intended by using `likely`/`unlikely`.
pnkfelix added a commit to pnkfelix/rust that referenced this pull request Jun 10, 2021
…pping`.

Test was added in PR rust-lang#84404.

The intent here is: The `copy`/`copy_overlapping` intrinsics are going through
some flip-flopping now of "are they intrinsics or not". We can achieve the same
effect that the test intended by using `likely`/`unlikely`.
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants