Make CTFE able to check for UB... #78407
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 60e41550266833277bb4f3fb94ddf2b2f5fa2fd8 with merge 3dbdd3b981f75f965ac04452739653a3d47ff0ed... |
|
☀️ Try build successful - checks-actions |
|
Queued 3dbdd3b981f75f965ac04452739653a3d47ff0ed with parent 35debd4, future comparison URL. |
|
Finished benchmarking try commit (3dbdd3b981f75f965ac04452739653a3d47ff0ed): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
|
Looking good. It affected the stress tests as expected (I even got the ballpark guess of a 100% regression about right), and only caused regressions in very small perf tests in incremental due to the additional query (all the other regressions are in the millisecond region). We even got perf improvements in the cases where we don't need |
lcnr
reviewed
Oct 27, 2020
oli-obk
force-pushed
the
ub_checkable_ctfe
branch
from
132b99c
to
13e63f2
Compare
camelid
added
the
S-waiting-on-author
|
(Added S-waiting-on-author so this isn't lost.) |
RalfJung
reviewed
Oct 31, 2020
RalfJung
reviewed
Oct 31, 2020
RalfJung
reviewed
Oct 31, 2020
RalfJung
reviewed
Oct 31, 2020
RalfJung
reviewed
Oct 31, 2020
RalfJung
reviewed
Oct 31, 2020
RalfJung
reviewed
Oct 31, 2020
crlf0710
added
A-const-eval
|
@oli-obk any updates? |
|
Nope, I'll try to find some time for it next week |
oli-obk
force-pushed
the
ub_checkable_ctfe
branch
from
13e63f2
to
874dfb8
Compare
oli-obk
added
S-waiting-on-review
oli-obk
force-pushed
the
ub_checkable_ctfe
branch
from
874dfb8
to
9d03206
Compare
|
Well... let's leave a note here:
These perf regressions permit us a path forward with |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors r=RalfJung,pnkfelix |
|
📌 Commit 53e3a23 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
This was referenced Jan 12, 2021
RalfJung
reviewed
Jan 13, 2021
| | | ||
| = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. | ||
| note: the lint level is defined here |
There was a problem hiding this comment.
@oli-obk any idea where all these lint changes are coming from? These tests were carefully crafted to exhibit certain code paths in the validator, and it seems like they are not hitting those code paths any more?
There was a problem hiding this comment.
well... they are not hitting those code paths anymore, because we optimize the code less, so the const evaluator directly hits the UB itself, thus we don't get to the validator anymore because we error out before that.
There was a problem hiding this comment.
That is very strange, why are optimizations needed to hit these code paths?
There was a problem hiding this comment.
Probably because of the reborrows? There are not that many differences between optimized mir and unoptimized mir: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=19dfcdc8284fd5eb7c9110a6ffcade88
There was a problem hiding this comment.
There was a problem hiding this comment.
Hm, yeah, reborrows would check dereferencability...
I am concerned about those code paths in the validator bitrotting, but I don't have an idea for what to do about it. We have to somehow make sure no reborrow is inserted after the transmute...
There was a problem hiding this comment.
I found a way to get back the old error: #81128
RalfJung
reviewed
Jan 13, 2021
| } | ||
| Ok(()) | ||
| }; | ||
| match tcx.hir().body_const_context(def_id.expect_local()) { |
There was a problem hiding this comment.
The expect_local call here means that write_mir_pretty no longer works for non local def_ids. The ability to print out MIR for non local functions this way is critical for the MIRAI project.
There was a problem hiding this comment.
Is the fix as simple as adding if let Some(local) = def_id.expect_local()? If so, are you interested in making a PR for that change?
There was a problem hiding this comment.
I doubt that the fix is that simple. I need a way of actually getting the MIR of non local functions. Perhaps use as_local and keep the old code for the None case?
There was a problem hiding this comment.
I think what we can do is to check is_const_fn_raw for the const fn path, and use tcx.instance_mir(ty::InstanceDef::Item(def_id)) for the other two paths
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Jun 16, 2021
Add a regression test for issue-76510 Fixed by rust-lang#78407, closes rust-lang#76510 r? `@oli-obk`
JohnTitor
added a commit
to JohnTitor/rust
that referenced
this pull request
Jun 16, 2021
Add a regression test for issue-76510 Fixed by rust-lang#78407, closes rust-lang#76510 r? ``@oli-obk``
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
... by not doing any optimizations on the
const fnMIR used in CTFE. This means we duplicate allconst fn's MIR now, once for CTFE, once for runtime. This PR is for checking the perf effect, so we have some data when talking about https://github.com/rust-lang/const-eval/blob/master/rfcs/0000-const-ub.mdTo do this, we now have two queries for obtaining mir:
optimized_mirandmir_for_ctfe. It is now illegal to invokeoptimized_mirto obtain the MIR of a const/static item's initializer, an array length, an inline const expression or an enum discriminant initializer. Forconst fn, bothoptimized_mirandmir_for_ctfework, the former returning the MIR that LLVM should use if the function is called at runtime. Similarly it is illegal to invokemir_for_ctfeon regular functions.This is all checked via appropriate assertions and I don't think it is easy to get wrong, as there should be no
mir_for_ctfecalls outside the const evaluator or metadata encoding. Almost all rustc devs should keep usingoptimized_mir(orinstance_mirfor that matter).