[WIP] Impl a more strict transmute check #51294
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Note: do not review the implementation approach too strictly – it is a quick and dirty implementation purely to check for how wide an impact on ecosystem would such a change have. |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/librustc/ty/sty.rs
Outdated
| None, | ||
| // Outright unspecified to be anything in particular | ||
| TyClosure(_, _) | TyGenerator(_, _, _) | TyGeneratorWitness(_) => Some(false), | ||
| TyFnDef(_, _) => Some(false), |
There was a problem hiding this comment.
Aren't function definitions guaranteed to be zst? (Not that it's useful to transmute them... but still)
There was a problem hiding this comment.
I don't recall us specifying that at any point (and in fact, I remember us debating whether we should make fndef types be a pointer in the past).
Either way, I’m willing to filter out those cases, if they occur on a crater run, manually.
There was a problem hiding this comment.
We had a warning period when we made them ZSTs, see also E0591 in intrinsicck.
I think Some(false) here is decent.
src/librustc/ty/sty.rs
Outdated
| let field_ty = field.ty(tcx, substs); | ||
| match field_ty.has_specified_layout(tcx) { | ||
| Some(true) => continue, | ||
| x => return x, |
There was a problem hiding this comment.
None should probably continue, too, but produce None in the end if no Some(false) was encountered before. Otherwise the field order can silence the lint
nagisa
force-pushed
the
transmute-lint
branch
3 times, most recently
from
7640c81
to
95c58be
Compare
pietroalbini
added
S-waiting-on-crater
|
@bors try |
[WIP] Impl a more strict transmute check Requesting a crater run to see the potential impact of forbidding transmutes to and from types with unspecified layouts.
|
☀️ Test successful - status-travis |
|
build-only crater run started on |
|
@pietroalbini Has the crater run been completed yet? |
|
@nagisa nope, it's running and it should finish in a few days. |
src/librustc/ty/sty.rs
Outdated
| // FIXME: not for fat pointers. | ||
| TyRawPtr(_) => Some(true), | ||
|
|
||
| // FIXME: Properly handle sizedness check |
There was a problem hiding this comment.
You can steal the sizedness check from layout code, I think it's pretty short.
src/librustc/ty/sty.rs
Outdated
| match self.sty { | ||
| // Not concrete, cannot say anything | ||
| TyInfer(_) | TyError | TyParam(_) | TyProjection(_) | TyAnon(_, _) | TyDynamic(_, _) => | ||
| None, |
There was a problem hiding this comment.
TyDynamic is a dyn Trait (trait object), so it should be treated like TyStr IMO.
src/librustc/ty/sty.rs
Outdated
| @@ -1848,6 +1848,78 @@ impl<'a, 'gcx, 'tcx> TyS<'tcx> { | |||
| _ => bug!("cannot convert type `{:?}` to a closure kind", self), | |||
| } | |||
| } | |||
|
|
|||
| // | |||
| pub fn has_specified_layout(&self, tcx: TyCtxt<'a, 'gcx, 'tcx>) -> Option<bool> { | |||
There was a problem hiding this comment.
IMO this should go in intrinsicck and preferably try to resolve projections, like layout code does.
|
Hi @nagisa (crater requester), @eddyb (PR reviewer)! Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-51294/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file. (interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious) |
pietroalbini
added
S-waiting-on-review
src/librustc/middle/intrinsicck.rs
Outdated
|
|
||
| if to.has_specified_layout(self.tcx) == Some(false) { | ||
| unspecified_layout("transmutation to a type with an unspecified layout", to); | ||
| } |
There was a problem hiding this comment.
Wait, this is below the "same size" check (see the return on line 80), is it intentional that it will only trigger if there's a size mismatch?
There was a problem hiding this comment.
Not intentional, I didn’t notice it.
That likely invalidates the crater run.
|
Only one crate regressed, and it looks unrelated:
|
|
I’m working on a fixed implementation at the moment (since @eddyb did found a pretty critical flaw in the implementation that may invalidate the crater results). |
|
☔ The latest upstream changes (presumably #51580) made this pull request unmergeable. Please resolve the merge conflicts. |
stokhos
added
S-waiting-on-author
|
ping from triage, @nagisa we haven't hear from you for awhile, will you have time to work on this again? |
This checks whether types being transmuted from and to have a specified layout.
|
@bors try requesting a crater run. check-only is fine. |
|
⌛ Trying commit 18d1459 with merge 5298b81bebd26bb9f87d5f99764aab01595b3f69... |
|
So far from the compiler codebase following problematic cases were seen:
|
pietroalbini
added
S-waiting-on-crater
|
☀️ Test successful - status-travis |
|
Crater run started on |
|
Hi @nagisa (crater requester), @eddyb (PR reviewer)! Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-51294/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file. (interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious) |
|
And congrats for breaking almost half of crates.io! |
That's why such experiments are better implemented as deny-by-default lints, so only root regressions show up. |
pietroalbini
added
S-waiting-on-author
|
@petrochenkov a good insight. If I happen to make another experiment, I will make sure that deny-by-default lint is used. It will also not hide failures if the crate is not checked due to depending on the failing crate. I did some very basic (automated) analysis on the log files. Sorted by impact, these are the "unspecified" layout types that are transmuted the most from or to:
|
|
cc @nikomatsakis you may be interested in analysis above. |
|
Closing the PR and moving the summary to the issue. |
This crate (among others) did'nt compile with an experimental [more strict transmute check](rust-lang/rust#51294) Hare's the error messages: (from http://cargobomb-reports.s3.amazonaws.com/pr-51294/5298b81bebd26bb9f87d5f99764aab01595b3f69-alt/reg/encode_unicode-0.3.1/log.txt) Jul 05 13:02:02.884 INFO kablam! error[E0912]: transmutation to a type with an unspecified layout Jul 05 13:02:02.884 INFO kablam! --> src/utf8_char.rs:283:16 Jul 05 13:02:02.884 INFO kablam! | Jul 05 13:02:02.884 INFO kablam! 283 | Ok(transmute(unused_zeroed)) Jul 05 13:02:02.884 INFO kablam! | ^^^^^^^^^ Jul 05 13:02:02.884 INFO kablam! | Jul 05 13:02:02.884 INFO kablam! = note: utf8_char::Utf8Char has an unspecified layout Jul 05 13:02:02.884 INFO kablam! Jul 05 13:02:02.884 INFO kablam! error[E0912]: transmutation from a type with an unspecified layout Jul 05 13:02:02.884 INFO kablam! --> src/utf8_iterator.rs:25:41 Jul 05 13:02:02.884 INFO kablam! | Jul 05 13:02:02.884 INFO kablam! 25 | let used = u32::from_le(unsafe{ mem::transmute(uc) }); Jul 05 13:02:02.884 INFO kablam! | ^^^^^^^^^^^^^^ Jul 05 13:02:02.884 INFO kablam! | Jul 05 13:02:02.884 INFO kablam! = note: utf8_char::Utf8Char has an unspecified layout Jul 05 13:02:02.884 INFO kablam! Jul 05 13:02:02.893 INFO kablam! error: aborting due to 2 previous errors
Requesting a crater run to see the potential impact of forbidding transmutes to and from types with unspecified layouts.