InvalidUndefBytes: Track size of undef region used #71610

divergentdave · 2020-04-27T14:46:13Z

This PR adds a size to UndefinedBehaviorInfo::InvalidUndefBytes, to keep track of how many undefined bytes in a row were accessed, and changes a few methods to pass this information through. This additional information will eventually be used in Miri to improve diagnostics for this UB error. See also rust-lang/miri#1354 for prior discussion.

I expect Miri will break the next time its submodule is updated, due to this change to the InvalidUndefBytes. (The current commit for src/tools/miri predates rust-lang/miri#1354, and thus does not try to destructure the InvalidUndefBytes variant) I have a corresponding change prepared for that repository.

r? @RalfJung

src/librustc_middle/mir/interpret/error.rs

src/librustc_middle/mir/interpret/value.rs

src/librustc_mir/interpret/validity.rs

RalfJung · 2020-04-27T15:46:31Z

One thing I am wondering about in terms of the Miri diagnostics that will be based on this: is "first uninit range" really the most useful piece of information? I could imagine it would be more useful to learn what the full memory range of the access was, and there in that access the uninit stuff is.

@divergentdave you seem to have a good use for these diagnostics as you initially contributed them, what do you think?

RalfJung · 2020-05-08T20:35:09Z

@divergentdave this PR needs rebasing to resolve conflicts and there are some review comments to take care of. No pressure though, if you need more time that's all right. :)

src/librustc_middle/mir/interpret/allocation.rs

divergentdave · 2020-05-14T14:40:32Z

I rebased everything up and changed the InvalidUninitBytes variant to hold an Option of a new struct, which now holds pointers and sizes for both the original memory access and the subset that was uninitialized.

src/librustc_middle/mir/interpret/error.rs

RalfJung · 2020-05-15T08:15:25Z

src/librustc_middle/mir/interpret/error.rs

@@ -384,7 +397,7 @@ pub enum UndefinedBehaviorInfo<'tcx> {
    /// Using a string that is not valid UTF-8,
    InvalidStr(std::str::Utf8Error),
    /// Using uninitialized data where it is not allowed.
-    InvalidUninitBytes(Option<Pointer>),
+    InvalidUninitBytes(Option<Box<UninitBytesAccess>>),


To avoid the size hit, boxing certainly makes sense. But OTOH we have a allocates function below to make sure that errors coming up during ConstProp do not allocate as that would be a perf hit. But I think this error here actually can come up during ConstProp so we probably cannot add it there. @oli-obk what do you think?

I updated allocates() and the tests still pass. It seems like InvalidUninitBytes(None) from reading a Scalar is far more common, and the Some case comes up with things like FFI with libc. Perhaps that means it doesn't come up in ConstProp?

Hm... that might be a coincidence though...

RalfJung · 2020-05-15T08:15:49Z

src/librustc_middle/mir/interpret/error.rs

+                but {} byte{} {} uninitialized starting at {}, \
+                and this operation requires initialized memory",


Suggested change

but {} byte{} {} uninitialized starting at {}, \

and this operation requires initialized memory",

but {} byte{} {} uninitialized starting at {}, \

and this operation requires initialized memory",

RalfJung · 2020-05-15T16:43:32Z

src/librustc_middle/mir/interpret/error.rs

+}
+
+#[cfg(target_arch = "x86_64")]
+static_assert_size!(UndefinedBehaviorInfo<'_>, 32);


Actually what I meant is doing this for InterpError. We don't really care about the individual variants.

Whoops, got it

RalfJung · 2020-05-15T18:31:51Z

src/librustc_middle/mir/interpret/error.rs

@@ -556,6 +577,9 @@ impl dyn MachineStopType {
    }
 }

+#[cfg(target_arch = "x86_64")]
+static_assert_size!(InterpError<'_>, 40);


Uff, 40 bytes. @oli-obk we might have to work on reducing that before @nnethercote catches us.^^

RalfJung · 2020-05-22T09:30:12Z

Looking good, thanks!
@bors r+

bors · 2020-05-22T09:30:13Z

📌 Commit 0148a7f has been approved by RalfJung

@ghost

Rollup of 4 pull requests Successful merges: - rust-lang#71610 (InvalidUndefBytes: Track size of undef region used) - rust-lang#72161 (Replace fcntl-based file lock with flock) - rust-lang#72306 (Break tokens before checking if they are 'probably equal') - rust-lang#72325 (Always generated object code for `#![no_builtins]`) Failed merges: r? @ghost

bors · 2020-05-22T14:48:33Z

⌛ Testing commit 0148a7f with merge a9ca1ec...

InvalidUndefBytes: Update to match rustc changes This is a companion PR for rust-lang/rust#71610. This won't build yet, but we may need these changes in a future rustup.

rust-highfive assigned RalfJung Apr 27, 2020

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

divergentdave mentioned this pull request Apr 27, 2020

InvalidUndefBytes: Update to match rustc changes rust-lang/miri#1374

Merged

oli-obk reviewed Apr 27, 2020

View reviewed changes

src/librustc_middle/mir/interpret/error.rs Outdated Show resolved Hide resolved

oli-obk reviewed Apr 27, 2020

View reviewed changes

src/librustc_middle/mir/interpret/value.rs Outdated Show resolved Hide resolved

oli-obk reviewed Apr 27, 2020

View reviewed changes

src/librustc_mir/interpret/validity.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

RalfJung added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 3, 2020

divergentdave force-pushed the InvalidUndefBytes-range branch from 8e28d0b to 3a12f2d Compare May 14, 2020 14:36

oli-obk reviewed May 14, 2020

View reviewed changes

src/librustc_middle/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

oli-obk reviewed May 14, 2020

View reviewed changes

src/librustc_middle/mir/interpret/allocation.rs Outdated Show resolved Hide resolved

divergentdave force-pushed the InvalidUndefBytes-range branch from 3a12f2d to 1c04c6d Compare May 14, 2020 14:42

RalfJung reviewed May 14, 2020

View reviewed changes

src/librustc_middle/mir/interpret/error.rs Show resolved Hide resolved

RalfJung reviewed May 14, 2020

View reviewed changes

src/librustc_middle/mir/interpret/error.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 14, 2020

View reviewed changes

src/librustc_middle/mir/interpret/error.rs Outdated Show resolved Hide resolved

divergentdave force-pushed the InvalidUndefBytes-range branch from 1c04c6d to 8f77f60 Compare May 15, 2020 04:35

RalfJung reviewed May 15, 2020

View reviewed changes

divergentdave force-pushed the InvalidUndefBytes-range branch from 8f77f60 to 9d56dac Compare May 15, 2020 16:41

RalfJung reviewed May 15, 2020

View reviewed changes

InvalidUninitBytes: Track more info about access

0148a7f

divergentdave force-pushed the InvalidUndefBytes-range branch from 9d56dac to 0148a7f Compare May 15, 2020 16:54

RalfJung reviewed May 15, 2020

View reviewed changes

RalfJung added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 22, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2020

RalfJung mentioned this pull request May 22, 2020

Rollup of 4 pull requests #72460

Merged

bors merged commit 2059112 into rust-lang:master May 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

InvalidUndefBytes: Track size of undef region used #71610

InvalidUndefBytes: Track size of undef region used #71610

divergentdave commented Apr 27, 2020

RalfJung commented Apr 27, 2020

This comment has been minimized.

RalfJung commented May 8, 2020

divergentdave commented May 14, 2020

RalfJung May 15, 2020

divergentdave May 15, 2020

RalfJung May 15, 2020

RalfJung May 15, 2020

RalfJung May 15, 2020

divergentdave May 15, 2020

RalfJung May 15, 2020

RalfJung commented May 22, 2020

bors commented May 22, 2020

bors commented May 22, 2020

		but {} byte{} {} uninitialized starting at {}, \
		and this operation requires initialized memory",

InvalidUndefBytes: Track size of undef region used #71610

InvalidUndefBytes: Track size of undef region used #71610

Conversation

divergentdave commented Apr 27, 2020

RalfJung commented Apr 27, 2020

This comment has been minimized.

RalfJung commented May 8, 2020

divergentdave commented May 14, 2020

RalfJung May 15, 2020

Choose a reason for hiding this comment

divergentdave May 15, 2020

Choose a reason for hiding this comment

RalfJung May 15, 2020

Choose a reason for hiding this comment

RalfJung May 15, 2020

Choose a reason for hiding this comment

RalfJung May 15, 2020

Choose a reason for hiding this comment

divergentdave May 15, 2020

Choose a reason for hiding this comment

RalfJung May 15, 2020

Choose a reason for hiding this comment

RalfJung commented May 22, 2020

bors commented May 22, 2020

bors commented May 22, 2020