Don't try to force_ptr pointers to zsts #68088

oli-obk · 2020-01-10T10:23:51Z

This is required to fix miri after #67501 broke it. The reason only miri sees this is that it uses validation on values during interpretation and not just on the final value of constants, which never contain such values.

RalfJung · 2020-01-10T14:17:03Z

Why does the old logic not work any more? The old structure was more clear, not having to duplicate the walk_aggregate.

RalfJung · 2020-01-10T14:18:09Z

src/librustc_mir/interpret/validity.rs

+                    return self.walk_aggregate(op, fields.take(1));
+                }
+                // FIXME: generalize this check to also do the optimization for aggregates that have
+                // no padding and all bits legal. So basically to all PODs


AFAIK all C structs are PODs, even if they have padding.

RalfJung · 2020-01-10T14:19:14Z

src/librustc_mir/interpret/validity.rs

-                        }
-                        _ => false,
+            ty::Array(tys, ..) | ty::Slice(tys) => {
+                // This optimization applies for types that can hold arbitrary bytes (such as


The comment here is weird now... the optimization comes way later, after the default case, and the first optimization is only for ZST, not what this comment says.

I'd structure this roughly as follows:

first optimization: ZST elem type

second optimization: scalar elem type where all types are valid

general case

Not sure if you can make the code actually follow this structure, but IMO this makes most sense for the comments.

that would mean that the "scalar elem type where all types are valid" optimization is inside one big if-block with a tiny else block for the general case in the end. I tried to avoid that.

Yeah I know, that's not great either. Maybe make the check_bytes into a separate function?

RalfJung · 2020-01-10T14:22:42Z

src/librustc_mir/interpret/validity.rs

+                // validation logic is skipped for uninhabited types (which may be zero sized).
+                let elem_layout = self.ecx.layout_of(tys)?;
+                if elem_layout.is_zst() {
+                    // Always validate the first element according to the regular rules.


I find this comment (and the one above) confusing. What's happening here is:

We check for ZST.

All ZST values are the same, so we don't need to check every element of the array/slice, just the first.

oli-obk · 2020-01-10T15:26:07Z

Why does the old logic not work any more? The old structure was more clear, not having to duplicate the walk_aggregate.

Because the old structure did a match inside a match arm guard to figure out whether to enter the arm. Also I didn't want to recompute the layout of the elements twice (once for zst, once for Scalar)

RalfJung · 2020-01-10T15:30:42Z

What I meant is, why does this code need changing at all? The new PR title makes it sound like it has something to do with force_ptr. That would be this code, I guess:

                // Size is not 0, get a pointer.
                let ptr = self.ecx.force_ptr(mplace.ptr)?;

That comment seems just entirely bogus now, so shouldn't that use check_ptr_access instead? And then it should still just work for ZST?

oli-obk · 2020-01-10T15:32:42Z

We need an AllocId right below that force_ptr. Or do you mean to skip the rest of the logic in case we get one of the zsts?

RalfJung · 2020-01-10T15:43:42Z

If the optimizations are early-returns in that branch, we could just fall back to the generic implementation in case of a ZST.

But the ZST optimization you are adding here is also fine, even though it's more than the smallest possible bugfix.

oli-obk · 2020-01-10T15:44:34Z

the smallest possible bugfix.

did that now

src/librustc_mir/interpret/validity.rs

…be checked

RalfJung · 2020-01-13T15:46:39Z

src/librustc_mir/interpret/validity.rs

@@ -608,9 +608,14 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
                    return Ok(());
                }
                // This is the element type size.
-                let ty_size = self.ecx.layout_of(tys)?.size;
+                let layout = self.ecx.layout_of(tys)?;
+                // Empty tuples and fieldless structs (the only ZSTs that allow reaching this code)


This makes it very odd that we have them in the same match arm as integers, given that we handle them entirely separate... we could have a match arm for these ZST that just doesn't do anything, I think. But that is a pre-existing condition, so we can leave that for later and fix Miri now.

well.. with the check_mplace_access we additionally get a check ensuring that the zst is correctly aligned (due to the early abort we're not checking this anymore), so if we want to keep doing such checks we're going to have to either duplicate all the logic in a struct/tuple arm or we have to intermingle it here as I did earlier with check_mplace_access.

IIRC validity checking assumes that the place itself is aligned and dereferencable? For references, we check those before adding them to the TODO list.

We definitely assume it to be dereferencable:

rust/src/librustc_mir/interpret/validity.rs

Line 668 in 19b9b26

/// This function checks the data at `op`. `op` is assumed to cover valid memory if it

I don't think we check alignment elsewhere... and that's okay, we want to use this to validate packed structs as well!

RalfJung · 2020-01-13T15:46:52Z

@bors r+ p=1
(needed for toolstate fix)

bors · 2020-01-13T15:47:19Z

📌 Commit 19b9b26 has been approved by RalfJung

bors · 2020-01-13T17:39:07Z

⌛ Testing commit 19b9b26 with merge 31dd4f4...

@RalfJung

Don't try to force_ptr pointers to zsts r? @RalfJung cc @wesleywiser This is required to fix miri after #67501 broke it. The reason only miri sees this is that it uses validation on values during interpretation and not just on the final value of constants, which never contain such values.

bors · 2020-01-13T20:56:38Z

☀️ Test successful - checks-azure
Approved by: RalfJung
Pushing 31dd4f4 to master...

rust-highfive assigned RalfJung Jan 10, 2020

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

This comment has been minimized.

Sign in to view

oli-obk force-pushed the fix_miri branch from 2f7f874 to ee3696b Compare January 10, 2020 10:24

This comment has been minimized.

Sign in to view

oli-obk changed the title ~~Fix miri~~ Don't try to force_ptr pointers to zsts Jan 10, 2020

RalfJung reviewed Jan 10, 2020

View reviewed changes

oli-obk force-pushed the fix_miri branch from ee3696b to c584a8a Compare January 10, 2020 15:44

RalfJung reviewed Jan 13, 2020

View reviewed changes

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

Early abort validation of arrays of zsts because there is no data to …

19b9b26

…be checked

oli-obk force-pushed the fix_miri branch from c584a8a to 19b9b26 Compare January 13, 2020 15:12

RalfJung reviewed Jan 13, 2020

View reviewed changes

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 Jan 13, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 13, 2020

bors merged commit 19b9b26 into rust-lang:master Jan 13, 2020

RalfJung mentioned this pull request Jan 14, 2020

miri no longer builds after rust-lang/rust#68078 #68081

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't try to force_ptr pointers to zsts #68088

Don't try to force_ptr pointers to zsts #68088

oli-obk commented Jan 10, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

RalfJung commented Jan 10, 2020

RalfJung Jan 10, 2020

RalfJung Jan 10, 2020

RalfJung Jan 10, 2020

oli-obk Jan 10, 2020

RalfJung Jan 10, 2020

RalfJung Jan 10, 2020

oli-obk commented Jan 10, 2020

RalfJung commented Jan 10, 2020

oli-obk commented Jan 10, 2020

RalfJung commented Jan 10, 2020

oli-obk commented Jan 10, 2020

RalfJung Jan 13, 2020

oli-obk Jan 13, 2020

RalfJung Jan 13, 2020

RalfJung Jan 13, 2020 •

edited

Loading

RalfJung commented Jan 13, 2020 •

edited

Loading

bors commented Jan 13, 2020

bors commented Jan 13, 2020

bors commented Jan 13, 2020

Don't try to force_ptr pointers to zsts #68088

Don't try to force_ptr pointers to zsts #68088

Conversation

oli-obk commented Jan 10, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

RalfJung commented Jan 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Jan 10, 2020

RalfJung commented Jan 10, 2020

oli-obk commented Jan 10, 2020

RalfJung commented Jan 10, 2020

oli-obk commented Jan 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

RalfJung commented Jan 13, 2020 • edited Loading

bors commented Jan 13, 2020

bors commented Jan 13, 2020

bors commented Jan 13, 2020

oli-obk commented Jan 10, 2020 •

edited

Loading

RalfJung Jan 13, 2020 •

edited

Loading

RalfJung commented Jan 13, 2020 •

edited

Loading