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

mangling: mangle impl params w/ v0 scheme #75675

Merged
merged 2 commits into from
Oct 16, 2020

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented Aug 18, 2020

This PR modifies v0 symbol mangling to include all generic parameters from impl blocks (not just those used in the self type) - an alternative fix to #75326.

original:
   _RNCNvXCs4fqI2P2rA04_19impl_param_manglingINtB4_3FooppENtNtNtNtCsfnEnqCNU58Z_4core4iter6traits8iterator8Iterator4next0B4_
//        |------------ B4_ ----------------|
// _R (N C (N v (X (C ((s 4fqI2p2rA04_) 19impl_param_mangling)) (I (N t B4_ 3Foo) pp E) (N t (N t (N t (N t (C ((s fnEnqCNU58Z_) 4core)) 4iter) 6traits) 8iterator) 8Iterator)) 4next) 0) B4_

modified:
   _RNvXINICs4fqI2P2rA04_11issue_753260pppEINtB5_3FooppENtNtNtNtCsfnEnqCNU58Z_4core4iter6traits8iterator8Iterator4nextB5_
// _R (N v (X (I (N I (C ((s 4fqI2P2rA04_) 11issue_75326)) 0) ppp E) (I (N t B5_ 3Foo) pp E) (N t (N t (N t (N t (C ((s fnEnqCNU58Z_) 4core)) 4iter) 6traits) 8iterator) 8Iterator)) 4next) B5_
//            |     ^                                              |
//            |     |                                              |
//            |     new impl namespace                             |

Submitted as a draft as after some discussion w/ @eddyb, I'm going to do some investigation into (yet more alternative) changes to polymorphization that might remove the necessity for this.

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 18, 2020
@lcnr
Copy link
Contributor

lcnr commented Aug 18, 2020

Would this allow us to revert #75518?

@davidtwco
Copy link
Member Author

davidtwco commented Aug 18, 2020

Would this allow us to revert #75518?

Yeah, it could.

Didn't see these CI failures locally - seems like the mangling is still correct but the discriminants have changed. Fixed now, you can't use std/core types in these tests and have to normalize for bitness.

@tesuji
Copy link
Contributor

tesuji commented Aug 19, 2020

Do we need to update the v0 RFC and related references?

@davidtwco davidtwco force-pushed the symbol-mangling-impl-params branch 5 times, most recently from 2fdea0f to 78108f9 Compare August 19, 2020 13:47
@davidtwco davidtwco marked this pull request as ready for review August 20, 2020 10:44
@davidtwco
Copy link
Member Author

After some more experimentation, I've opened #75737 which removes predicate logic from polymorphization and depends on this PR. I think we should proceed with landing this.

@bors

This comment has been minimized.

@jyn514 jyn514 added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-codegen Area: Code generation labels Sep 15, 2020
Copy link
Member Author

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

Noting some things that need changed after chatting with @eddyb.

compiler/rustc_symbol_mangling/src/v0.rs Outdated Show resolved Hide resolved
compiler/rustc_symbol_mangling/src/test.rs Outdated Show resolved Hide resolved
@bors

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 Oct 15, 2020
@bors
Copy link
Contributor

bors commented Oct 15, 2020

⌛ Testing commit fbdfe2c with merge f676bae7da109140bd99bfc41f460ae74015b887...

@bors
Copy link
Contributor

bors commented Oct 15, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 15, 2020
@davidtwco
Copy link
Member Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Oct 15, 2020

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
  • There's another pull request that is currently being tested, blocking this pull request: No more target.target #77943

@bors
Copy link
Contributor

bors commented Oct 15, 2020

📌 Commit fbdfe2c has been approved by eddyb

@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 Oct 15, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 15, 2020
…ms, r=eddyb

mangling: mangle impl params w/ v0 scheme

This PR modifies v0 symbol mangling to include all generic parameters from impl blocks (not just those used in the self type) - an alternative fix to rust-lang#75326.

```
original:
   _RNCNvXCs4fqI2P2rA04_19impl_param_manglingINtB4_3FooppENtNtNtNtCsfnEnqCNU58Z_4core4iter6traits8iterator8Iterator4next0B4_
//        |------------ B4_ ----------------|
// _R (N C (N v (X (C ((s 4fqI2p2rA04_) 19impl_param_mangling)) (I (N t B4_ 3Foo) pp E) (N t (N t (N t (N t (C ((s fnEnqCNU58Z_) 4core)) 4iter) 6traits) 8iterator) 8Iterator)) 4next) 0) B4_

modified:
   _RNvXINICs4fqI2P2rA04_11issue_753260pppEINtB5_3FooppENtNtNtNtCsfnEnqCNU58Z_4core4iter6traits8iterator8Iterator4nextB5_
// _R (N v (X (I (N I (C ((s 4fqI2P2rA04_) 11issue_75326)) 0) ppp E) (I (N t B5_ 3Foo) pp E) (N t (N t (N t (N t (C ((s fnEnqCNU58Z_) 4core)) 4iter) 6traits) 8iterator) 8Iterator)) 4next) B5_
//            |     ^                                              |
//            |     |                                              |
//            |     new impl namespace                             |
```

~~Submitted as a draft as after some discussion w/ @eddyb, I'm going to do some investigation into (yet more alternative) changes to polymorphization that might remove the necessity for this.~~

r? @eddyb
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 15, 2020
…ms, r=eddyb

mangling: mangle impl params w/ v0 scheme

This PR modifies v0 symbol mangling to include all generic parameters from impl blocks (not just those used in the self type) - an alternative fix to rust-lang#75326.

```
original:
   _RNCNvXCs4fqI2P2rA04_19impl_param_manglingINtB4_3FooppENtNtNtNtCsfnEnqCNU58Z_4core4iter6traits8iterator8Iterator4next0B4_
//        |------------ B4_ ----------------|
// _R (N C (N v (X (C ((s 4fqI2p2rA04_) 19impl_param_mangling)) (I (N t B4_ 3Foo) pp E) (N t (N t (N t (N t (C ((s fnEnqCNU58Z_) 4core)) 4iter) 6traits) 8iterator) 8Iterator)) 4next) 0) B4_

modified:
   _RNvXINICs4fqI2P2rA04_11issue_753260pppEINtB5_3FooppENtNtNtNtCsfnEnqCNU58Z_4core4iter6traits8iterator8Iterator4nextB5_
// _R (N v (X (I (N I (C ((s 4fqI2P2rA04_) 11issue_75326)) 0) ppp E) (I (N t B5_ 3Foo) pp E) (N t (N t (N t (N t (C ((s fnEnqCNU58Z_) 4core)) 4iter) 6traits) 8iterator) 8Iterator)) 4next) B5_
//            |     ^                                              |
//            |     |                                              |
//            |     new impl namespace                             |
```

~~Submitted as a draft as after some discussion w/ @eddyb, I'm going to do some investigation into (yet more alternative) changes to polymorphization that might remove the necessity for this.~~

r? @eddyb
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 16, 2020
Rollup of 14 pull requests

Successful merges:

 - rust-lang#75023 (ensure arguments are included in count mismatch span)
 - rust-lang#75265 (Add `str::{Split,RSplit,SplitN,RSplitN,SplitTerminator,RSplitTerminator,SplitInclusive}::as_str` methods)
 - rust-lang#75675 (mangling: mangle impl params w/ v0 scheme)
 - rust-lang#76084 (Refactor io/buffered.rs into submodules)
 - rust-lang#76119 (Stabilize move_ref_pattern)
 - rust-lang#77493 (ICEs should always print the top of the query stack)
 - rust-lang#77619 (Use futex-based thread-parker for Wasm32.)
 - rust-lang#77646 (For backtrace, use StaticMutex instead of a raw sys Mutex.)
 - rust-lang#77648 (Static mutex is static)
 - rust-lang#77657 (Cleanup cloudabi mutexes and condvars)
 - rust-lang#77672 (Simplify doc-cfg rendering based on the current context)
 - rust-lang#77780 (rustc_parse: fix spans on cast and range exprs with attrs)
 - rust-lang#77935 (BTreeMap: make PartialCmp/PartialEq explicit and tested)
 - rust-lang#77980 (Fix intra doc link for needs_drop)

Failed merges:

r? `@ghost`
@bors bors merged commit 1643fd8 into rust-lang:master Oct 16, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 16, 2020
@davidtwco davidtwco deleted the symbol-mangling-impl-params branch October 16, 2020 10:26
davidtwco added a commit to davidtwco/rust that referenced this pull request Nov 2, 2020
This commit removes all logic which marks parameters as used based on
their presence in predicates - given rust-lang#75675, this will
enable more polymorphization and avoid the symbol clashes that predicate
logic previously sidestepped.

Signed-off-by: David Wood <david@davidtw.co>
davidtwco added a commit to davidtwco/rust that referenced this pull request Oct 4, 2021
This commit removes all logic which marks parameters as used based on
their presence in predicates - given rust-lang#75675, this will
enable more polymorphization and avoid the symbol clashes that predicate
logic previously sidestepped.

Signed-off-by: David Wood <david@davidtw.co>
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Oct 16, 2021
…edicates, r=lcnr

polymorphization: shims and predicates

Supersedes rust-lang#75737 and rust-lang#75414. This pull request includes up some changes to polymorphization which hadn't landed previously and gets stage2 bootstrapping and the test suite passing when polymorphization is enabled. There are still issues with `type_id` and polymorphization to investigate but this should get polymorphization in a reasonable state to work on.

- rust-lang#75737 and rust-lang#75414 both worked but were blocked on having the rest of the test suite pass (with polymorphization enabled) with and without the PRs. It makes more sense to just land these so that the changes are in.
- rust-lang#75737's changes remove the restriction of `InstanceDef::Item` on polymorphization, so that shims can now be polymorphized. This won't have much of an effect until polymorphization's analysis is more advanced, but it doesn't hurt.
- rust-lang#75414's changes remove all logic which marks parameters as used based on their presence in predicates - given rust-lang#75675, this will enable more polymorphization and avoid the symbol clashes that predicate logic previously sidestepped.
- Polymorphization now explicitly checks (and skips) foreign items, this is necessary for stage2 bootstrapping to work when polymorphization is enabled.
- The conditional determining the emission of a note adding context to a post-monomorphization error has been modified. Polymorphization results in `optimized_mir` running for shims during collection where that wouldn't happen previously, some errors are emitted during `optimized_mir` and these were considered post-monomorphization errors with the existing logic (more errors and shims have a `DefId` coming from the std crate, not the local crate), adding a note that resulted in tests failing. It isn't particularly feasible to change where polymorphization runs or prevent it from using `optimized_mir`, so it seemed more reasonable to not change the conditional.
- `characteristic_def_id_of_type` was being invoked during partitioning for self types of impl blocks which had projections that depended on the value of unused generic parameters of a function - this caused a ICE in a debuginfo test. If partitioning is enabled and the instance needs substitution then this is skipped. That test still fails for me locally, but not with an ICE, but it fails in a fresh checkout too, so 🤷‍♂️.

r? `@lcnr`
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 17, 2021
…icates, r=lcnr

polymorphization: shims and predicates

Supersedes rust-lang#75737 and rust-lang#75414. This pull request includes up some changes to polymorphization which hadn't landed previously and gets stage2 bootstrapping and the test suite passing when polymorphization is enabled. There are still issues with `type_id` and polymorphization to investigate but this should get polymorphization in a reasonable state to work on.

- rust-lang#75737 and rust-lang#75414 both worked but were blocked on having the rest of the test suite pass (with polymorphization enabled) with and without the PRs. It makes more sense to just land these so that the changes are in.
- rust-lang#75737's changes remove the restriction of `InstanceDef::Item` on polymorphization, so that shims can now be polymorphized. This won't have much of an effect until polymorphization's analysis is more advanced, but it doesn't hurt.
- rust-lang#75414's changes remove all logic which marks parameters as used based on their presence in predicates - given rust-lang#75675, this will enable more polymorphization and avoid the symbol clashes that predicate logic previously sidestepped.
- Polymorphization now explicitly checks (and skips) foreign items, this is necessary for stage2 bootstrapping to work when polymorphization is enabled.
- The conditional determining the emission of a note adding context to a post-monomorphization error has been modified. Polymorphization results in `optimized_mir` running for shims during collection where that wouldn't happen previously, some errors are emitted during `optimized_mir` and these were considered post-monomorphization errors with the existing logic (more errors and shims have a `DefId` coming from the std crate, not the local crate), adding a note that resulted in tests failing. It isn't particularly feasible to change where polymorphization runs or prevent it from using `optimized_mir`, so it seemed more reasonable to not change the conditional.
- `characteristic_def_id_of_type` was being invoked during partitioning for self types of impl blocks which had projections that depended on the value of unused generic parameters of a function - this caused a ICE in a debuginfo test. If partitioning is enabled and the instance needs substitution then this is skipped. That test still fails for me locally, but not with an ICE, but it fails in a fresh checkout too, so 🤷‍♂️.

r? `@lcnr`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation 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.

8 participants