validate basic sanity for TerminatorKind #72810
Conversation
rust-highfive
added
the
S-waiting-on-review
RalfJung
force-pushed
the
mir-terminate-sanity
branch
from
37676c2
to
5707838
Compare
| self.check_bb(terminator.source_info.span, *drop); | ||
| } | ||
| } | ||
| TerminatorKind::FalseEdges { real_target, imaginary_target } => { |
There was a problem hiding this comment.
Btw, does anyone know why this is named in plural? Shouldn't it be FalseEdge instead?
Cc @matthewjasper
There was a problem hiding this comment.
I think that it may have been accurate in the past. Feel free to change it.
RalfJung
force-pushed
the
mir-terminate-sanity
branch
from
24578ea
to
e07e424
Compare
RalfJung
force-pushed
the
mir-terminate-sanity
branch
from
9a78edd
to
f793c0b
Compare
|
@bors r+ |
|
📌 Commit f793c0b has been approved by |
bors
added
S-waiting-on-bors
…nas-schievink validate basic sanity for TerminatorKind r? @jonas-schievink This mainly checks that all `BasicBlock` actually exist. On top of that, it checks that `Call` actually calls something of `FnPtr`/`FnDef` type, and `Assert` has to work on a `bool`. Also `SwitchInt` cannot have an empty target list.
| self.check_bb(location, *unwind); | ||
| } | ||
| } | ||
| TerminatorKind::Call { func, destination, cleanup, .. } => { |
There was a problem hiding this comment.
Call (and perhaps others?) could also validate that the destination doesn't overlap with any of the arguments. This would have caught a miscompilation in #72632.
There was a problem hiding this comment.
DropAndReplace (and Yield?) can do the same check. Not sure about inline assembly, but it seems like that's a candidate for such a check as well.
There was a problem hiding this comment.
Hm, now I wonder if Miri would have caught that (for Call). Unfortunately we can't test Miri on MIR that rustc does not generate.
There was a problem hiding this comment.
It probably would have, but I haven't yet figured out how to use a locally built miri with external code (like with cargo-miri).
There was a problem hiding this comment.
It probably would have, but I haven't yet figured out how to use a locally built miri with external code (like with cargo-miri).
I usually do ./miri install and then use cargo miri.
There was a problem hiding this comment.
./src/tools/miri/miri install fails with "can't find crate for rustc_apfloat" for me, and running the miri tests ICEs the compiler
There was a problem hiding this comment.
Ah I've only ever used the script in a standalone clone... yeah it probably doesn't work in the rustc folder layout.
running the miri tests ICEs the compiler
How do you run them? ./x.py test --stage 0 src/tools/miri should work -- except when the Miri toolstate is broken for the commit you're basing your work on.
There was a problem hiding this comment.
Yeah that's what I tried to run, but it enters unreachable code while decoding or encoding something. Not really sure what's happening, but I'll do more digging.
There was a problem hiding this comment.
That sounds like an incremental problem or --keep-stage gone wrong... strange.
…nas-schievink validate basic sanity for TerminatorKind r? @jonas-schievink This mainly checks that all `BasicBlock` actually exist. On top of that, it checks that `Call` actually calls something of `FnPtr`/`FnDef` type, and `Assert` has to work on a `bool`. Also `SwitchInt` cannot have an empty target list.
oli-obk
added
the
A-mir
Rollup of 5 pull requests Successful merges: - rust-lang#72810 (validate basic sanity for TerminatorKind) - rust-lang#72989 (Revert pr 71840) - rust-lang#72993 (Count the beta prerelease number just from master) - rust-lang#73057 (Clean up E0644 explanation) - rust-lang#73059 (remove outdated comment) Failed merges: r? @ghost
r? @jonas-schievink
This mainly checks that all
BasicBlockactually exist. On top of that, it checks thatCallactually calls something ofFnPtr/FnDeftype, andAsserthas to work on abool. AlsoSwitchIntcannot have an empty target list.