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

rustc: don't resolve Instances which would produce malformed shims. #69036

Merged
merged 2 commits into from
Mar 19, 2020

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Feb 10, 2020

There are some InstanceDef variants (shims and drop "glue") which contain a Ty, and that Ty is used in generating the shim MIR. But if that Ty mentions any generic parameters, the generated shim would refer to them (but they won't match the Substs of the Instance), or worse, generating the shim would fail because not enough of the type is known.

Ideally we would always produce a "skeleton" of the type, e.g. (_, _) for dropping any tuples with two elements, or Vec<_> for dropping any Vec value, but that's a lot of work, and they would still not match the Substs of the Instance as it exists today, so Instance would probably need to change.

By making Instance::resolve return None in the still-generic cases, we get behavior similar to specialization, where a default can only be used if there are no more generic parameters which would allow a more specialized impl to match.


This was found while testing the MIR inliner with #68965, because it was trying to inline shims.

cc @rust-lang/wg-mir-opt

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(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 Feb 10, 2020
@eddyb
Copy link
Member Author

eddyb commented Feb 10, 2020

Just in case this is the cause of the regression at #68965 (comment):
@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Feb 10, 2020

⌛ Trying commit 2d03e10 with merge e369f6b...

bors added a commit that referenced this pull request Feb 10, 2020
rustc: don't resolve Instances which would produce malformed shims.

There are some `InstanceDef` variants (shims and drop "glue") which contain a `Ty`, and that `Ty` is used in generating the shim MIR. But if that `Ty` mentions any generic parameters, the generated shim would refer to them (but they won't match the `Substs` of the `Instance`), or worse, generating the shim would fail because not enough of the type is known.

Ideally we would always produce a "skeleton" of the type, e.g. `(_, _)` for dropping any tuples with two elements, or `Vec<_>` for dropping any `Vec` value, but that's a lot of work, and they would still not match the `Substs` of the `Instance` as it exists today, so `Instance` would probably need to change.

By making `Instance::resolve` return `None` in the still-generic cases, we get behavior similar to specialization, where a default can only be used if there are no more generic parameters which would allow a more specialized `impl` to match.

<hr/>

This was found while testing the MIR inliner with #68965, because it was trying to inline shims.

cc @rust-lang/wg-mir-opt
@wesleywiser
Copy link
Member

r? @wesleywiser

@Mark-Simulacrum
Copy link
Member

@bors r- try- try

Attempting to convince bors of reality...

@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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 10, 2020
@Mark-Simulacrum
Copy link
Member

@rust-timer build e369f6b

Seems bors flaked

@bors try-

@Mark-Simulacrum
Copy link
Member

@rust-timer build e369f6b

@rust-timer
Copy link
Collaborator

Queued e369f6b with parent 4d1241f, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit e369f6b, comparison URL.

@eddyb
Copy link
Member Author

eddyb commented Feb 11, 2020

Okay that's pure noise :(
Does not make me feel good about relying on perf.rlo

src/librustc/ty/instance.rs Outdated Show resolved Hide resolved
src/librustc/ty/instance.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Feb 12, 2020

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

@joelpalmer
Copy link

Triaged

@wesleywiser
Copy link
Member

@eddyb Please let me know if there's anything I can do to help get this merged 🙂

@eddyb

This comment has been minimized.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 11, 2020
@bors

This comment has been minimized.

@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 Mar 14, 2020
@eddyb
Copy link
Member Author

eddyb commented Mar 15, 2020

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Mar 15, 2020

📌 Commit bc8ff3f has been approved by nikomatsakis

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 15, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 17, 2020
rustc: don't resolve Instances which would produce malformed shims.

There are some `InstanceDef` variants (shims and drop "glue") which contain a `Ty`, and that `Ty` is used in generating the shim MIR. But if that `Ty` mentions any generic parameters, the generated shim would refer to them (but they won't match the `Substs` of the `Instance`), or worse, generating the shim would fail because not enough of the type is known.

Ideally we would always produce a "skeleton" of the type, e.g. `(_, _)` for dropping any tuples with two elements, or `Vec<_>` for dropping any `Vec` value, but that's a lot of work, and they would still not match the `Substs` of the `Instance` as it exists today, so `Instance` would probably need to change.

By making `Instance::resolve` return `None` in the still-generic cases, we get behavior similar to specialization, where a default can only be used if there are no more generic parameters which would allow a more specialized `impl` to match.

<hr/>

This was found while testing the MIR inliner with rust-lang#68965, because it was trying to inline shims.

cc @rust-lang/wg-mir-opt
Centril added a commit to Centril/rust that referenced this pull request Mar 19, 2020
rustc: don't resolve Instances which would produce malformed shims.

There are some `InstanceDef` variants (shims and drop "glue") which contain a `Ty`, and that `Ty` is used in generating the shim MIR. But if that `Ty` mentions any generic parameters, the generated shim would refer to them (but they won't match the `Substs` of the `Instance`), or worse, generating the shim would fail because not enough of the type is known.

Ideally we would always produce a "skeleton" of the type, e.g. `(_, _)` for dropping any tuples with two elements, or `Vec<_>` for dropping any `Vec` value, but that's a lot of work, and they would still not match the `Substs` of the `Instance` as it exists today, so `Instance` would probably need to change.

By making `Instance::resolve` return `None` in the still-generic cases, we get behavior similar to specialization, where a default can only be used if there are no more generic parameters which would allow a more specialized `impl` to match.

<hr/>

This was found while testing the MIR inliner with rust-lang#68965, because it was trying to inline shims.

cc @rust-lang/wg-mir-opt
bors added a commit that referenced this pull request Mar 19, 2020
Rollup of 9 pull requests

Successful merges:

 - #69036 (rustc: don't resolve Instances which would produce malformed shims.)
 - #69443 (tidy: Better license checks.)
 - #69814 (Smaller and more correct generator codegen)
 - #69929 (Regenerate tables for Unicode 13.0.0)
 - #69959 (std: Don't abort process when printing panics in tests)
 - #69969 (unix: Set a guard page at the end of signal stacks)
 - #70005 ([rustdoc] Improve visibility for code blocks warnings)
 - #70088 (Use copy bound in atomic operations to generate simpler MIR)
 - #70095 (Implement -Zlink-native-libraries)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Mar 19, 2020
Rollup of 9 pull requests

Successful merges:

 - #68941 (Properly handle Spans that reference imported SourceFiles)
 - #69036 (rustc: don't resolve Instances which would produce malformed shims.)
 - #69443 (tidy: Better license checks.)
 - #69814 (Smaller and more correct generator codegen)
 - #69929 (Regenerate tables for Unicode 13.0.0)
 - #69959 (std: Don't abort process when printing panics in tests)
 - #69969 (unix: Set a guard page at the end of signal stacks)
 - #70005 ([rustdoc] Improve visibility for code blocks warnings)
 - #70088 (Use copy bound in atomic operations to generate simpler MIR)

Failed merges:

r? @ghost
@bors bors merged commit ffb3c2c into rust-lang:master Mar 19, 2020
@eddyb eddyb deleted the monoshim branch March 19, 2020 09:18
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.

10 participants