Recursively transform predicates in ty::Dynamic rather than using identity
#123045
Conversation
rustbot
added
PG-exploit-mitigations
|
Some changes occurred in compiler/rustc_symbol_mangling/src/typeid cc @rust-lang/project-exploit-mitigations, @rcvalle |
|
Is there a way to bulk fix the |
compiler-errors
force-pushed
the
no-id-dyn
branch
from
16c9754
to
4ed0325
Compare
|
@rcvalle: Specifically, I don't understand what you mean in #116404 (comment). If you disagree w/ this approach, I would appreciate if you constructed a test example that SIGILLs after this PR and not before (or at least describe such a case). |
|
I agree with this approach. I just didn't know it was possible/how to do it at the time I implemented it using the identity of the trait that implemented the method (see also my comment on #123012 (comment)). |
|
This looks reasonable to me as well. |
Sorry, I've been creating and maintaining them manually 😞. One thing that might help is to look at the .ll file to get the correct encoding, double check it, and put it in the tests (if you're not doing this already). |
|
Ok well it's gonna take approximately 5 millennia to bless those tests then 💀 |
|
No worries! I can work on that and send a PR later. |
|
This raised a thought in my mind - once we're done getting everything working, should |
|
Yes, it could be implemented as a |
|
I don't know, maybe? If we can do depth-first transformation of types using it, I guess it should work. If it's an improvement, I'm definitively up to it later. |
|
FWIW, I don't know how type visitor works in detail, but how we do it now at least forces us to explicitly maintain the types we support encoding for, and are let known otherwise. |
|
I strongly agree that this should be rewritten as a |
| }) | ||
| }) | ||
| .collect(); | ||
| predicates.sort_by(|a, b| a.skip_binder().stable_cmp(tcx, &b.skip_binder())); |
There was a problem hiding this comment.
why is this needed? please add a comment why we have to sort here
There was a problem hiding this comment.
Because it transforms types to other types, which possibly change their stable order.
There was a problem hiding this comment.
Actually, this doesn't matter. We only sort by def path rn lol
|
Some changes occurred in tests/codegen/sanitizer cc @rust-lang/project-exploit-mitigations, @rcvalle |
compiler-errors
force-pushed
the
no-id-dyn
branch
from
53a2744
to
04743a8
Compare
|
☔ The latest upstream changes (presumably #123455) made this pull request unmergeable. Please resolve the merge conflicts. |
|
this pr is stuck in review hell and @maurer has volunteered to just do it themselves and i'm just gonna r+ that |
It is not correct to be using the identity trait ref for a
dyntype. This leads to us downstream not being able to assert the non-existence of things likety::Alias,ty::Param,ty::ReEarlyParam, etc. and also doesn't allow us to distinguish, e.g.,dyn Trait<i32>anddyn Trait<&str>, lol.Instead, just recursively transform the
dynpredicates. We also should definitely be keeping around the projection predicates too.cc @rcvalle @maurer