mangling: mangle impl params w/ v0 scheme #75675

davidtwco · 2020-08-18T18:31:29Z

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

lcnr · 2020-08-18T20:39:20Z

Would this allow us to revert #75518?

davidtwco · 2020-08-18T20:40:27Z

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 · 2020-08-19T08:11:33Z

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

davidtwco · 2020-08-20T10:47:02Z

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.

src/librustc_symbol_mangling/v0.rs

src/test/ui/symbol-names/impl1.legacy.stderr

src/test/ui/symbol-names/impl1.rs

src/test/ui/symbol-names/issue-60925.rs

src/test/ui/symbol-names/issue-75326.rs

davidtwco

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

compiler/rustc_symbol_mangling/src/v0.rs

compiler/rustc_symbol_mangling/src/test.rs

bors · 2020-10-15T12:31:33Z

⌛ Testing commit fbdfe2c with merge f676bae7da109140bd99bfc41f460ae74015b887...

bors · 2020-10-15T16:46:33Z

💥 Test timed out

davidtwco · 2020-10-15T16:52:40Z

@bors r=eddyb

bors · 2020-10-15T16:52:42Z

💡 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 · 2020-10-15T16:52:42Z

📌 Commit fbdfe2c has been approved by eddyb

@eddyb

…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

@eddyb

…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

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`

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>

…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`

…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`

rust-highfive assigned eddyb Aug 18, 2020

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

davidtwco force-pushed the symbol-mangling-impl-params branch 5 times, most recently from 2fdea0f to 78108f9 Compare August 19, 2020 13:47

davidtwco marked this pull request as ready for review August 20, 2020 10:44

davidtwco mentioned this pull request Aug 20, 2020

polymorphize: remove predicate logic #75737

Closed