Miri visitor: detect primitive types based on type, not layout (also, more tests) #69646

RalfJung · 2020-03-02T20:17:56Z

I also converted the union-based transmutes to use mem::transmute for increased readability.

r? @eddyb @oli-obk

src/librustc_mir/interpret/validity.rs

eddyb

LGTM modulo nits (mostly comments, really).

RalfJung · 2020-03-02T21:22:38Z

src/librustc_mir/interpret/validity.rs

+            | ty::Dynamic(..)
+            | ty::Closure(..)
+            | ty::Generator(..)
+            | ty::GeneratorWitness(..) => Ok(false),


GeneratorWitness also has a layout that we can traverse, right? I have no idea what that type kind dos (and its doc comment is pretty useless as well).

cc @Zoxc but I think it's meant like a builtin PhantomData.

A value cannot have the type GeneratorWitness. GeneratorWitness may only appear inside a ty::Generator. I think having it panic here would be the right choice.

GeneratorWitness may only appear inside a ty::Generator.

But doesn't that mean that a value of ty::Generator type might contain a GeneratorWitness?
The doc comment says something similar and I don't understand what "only appear inside a Generator means.

But I'll happily make it panic.

It's in the Substs of the Generator, but not as field type. This is why I mentioned PhantomData.

RalfJung · 2020-03-02T21:23:16Z

src/test/ui/consts/const-eval/ub-enum.rs

+enum UninhDiscriminant {
+    A,
+    B(!),
+    C,
+    D(Never),
+}


I hope this is the kind of enum that triggers interesting uninhabited variants?

Also see the ones with Result below.

RalfJung · 2020-03-02T21:25:13Z

@eddyb I think I took care of the comments, and I also pointed to some parts that I'd prefer you to double-check. :)

eddyb · 2020-03-02T22:38:58Z

@bors r+

bors · 2020-03-02T22:39:00Z

📌 Commit e4ae6e6566a572692537af665982d5421ed84662 has been approved by eddyb

bors · 2020-03-04T07:44:49Z

☔ The latest upstream changes (presumably #69550) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2020-03-04T08:12:18Z

Rebased. @bors r=eddyb

bors · 2020-03-04T08:12:20Z

📌 Commit bd4c9a167b05a45c9ca52213a57f16c0ba642646 has been approved by eddyb

…, really

@eddyb

Miri visitor: detect primitive types based on type, not layout (also, more tests) I also converted the union-based transmutes to use `mem::transmute` for increased readability. r? @eddyb @oli-obk

Centril · 2020-03-08T10:49:39Z

Failed in #69819 (comment), @bors r-

RalfJung · 2020-03-08T10:54:05Z

Argh, a 32bit-only failure... sorry for that!

RalfJung · 2020-03-08T11:23:33Z

@bors r=eddyb

bors · 2020-03-08T11:23:34Z

📌 Commit e623ea5350a22a670a1e4d9a838eb50f0acec9f7 has been approved by eddyb

bors · 2020-03-08T11:23:35Z

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

RalfJung · 2020-03-08T11:43:40Z

Tidy is merciless and almighty.
@bors r=eddyb

bors · 2020-03-08T11:43:41Z

📌 Commit a95f00f has been approved by eddyb

bors · 2020-03-08T11:43:42Z

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@eddyb

Miri visitor: detect primitive types based on type, not layout (also, more tests) I also converted the union-based transmutes to use `mem::transmute` for increased readability. r? @eddyb @oli-obk

@ghost

Rollup of 8 pull requests Successful merges: - #69631 (remove non-sysroot sources from rust-src component) - #69646 (Miri visitor: detect primitive types based on type, not layout (also, more tests)) - #69651 (Try to ensure usize marker does not get merged) - #69668 (More documentation and simplification of BTreeMap's internals) - #69685 (unix: Don't override existing SIGSEGV/BUS handlers) - #69771 (Cleanup E0390 explanation) - #69777 (Add missing ` in doc for File::with_options()) - #69812 (Refactorings to method/probe.rs and CrateId) Failed merges: r? @ghost

@ghost

Rollup of 7 pull requests Successful merges: - #69631 (remove non-sysroot sources from rust-src component) - #69646 (Miri visitor: detect primitive types based on type, not layout (also, more tests)) - #69651 (Try to ensure usize marker does not get merged) - #69668 (More documentation and simplification of BTreeMap's internals) - #69771 (Cleanup E0390 explanation) - #69777 (Add missing ` in doc for File::with_options()) - #69812 (Refactorings to method/probe.rs and CrateId) Failed merges: r? @ghost

@oli-obk

Ensure that validity only raises validity errors For now, only as a debug-assertion (similar to const-prop detecting errors that allocate). Now includes rust-lang#69646. [Relative diff](RalfJung/rust@layout-visitor...RalfJung:validity-errors). r? @oli-obk

@oli-obk

Ensure that validity only raises validity errors For now, only as a debug-assertion (similar to const-prop detecting errors that allocate). Now includes rust-lang#69646. [Relative diff](RalfJung/rust@layout-visitor...RalfJung:validity-errors). r? @oli-obk

@oli-obk

Ensure that validity only raises validity errors For now, only as a debug-assertion (similar to const-prop detecting errors that allocate). Now includes rust-lang#69646. [Relative diff](RalfJung/rust@layout-visitor...RalfJung:validity-errors). r? @oli-obk

@oli-obk

Ensure that validity only raises validity errors For now, only as a debug-assertion (similar to const-prop detecting errors that allocate). Now includes rust-lang#69646. [Relative diff](RalfJung/rust@layout-visitor...RalfJung:validity-errors). r? @oli-obk

cuviper · 2020-07-01T22:22:18Z

src/test/ui/consts/const-eval/ub-enum.stderr

+  --> $DIR/ub-enum.rs:92:1
+   |
+LL | const BAD_UNINHABITED_WITH_DATA2: Result<(i32, !), (i32, Never)> = unsafe { mem::transmute(1u64) };
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of uninhabited type Never at .<enum-variant(Err)>.0.1


Is it supposed to be deterministic that this fails on the Err variant?
On s390x, I'm seeing an error on the Ok variant instead for both of these Result tests.

I suppose it then also says ! instead of Never?

That is odd... it means that data layout is very different on s390x? Could this be an endianess thing?

Yeah, the test failure looks like this:

---- [ui] ui/consts/const-eval/ub-enum.rs stdout ---- diff of stderr: 90 --> $DIR/ub-enum.rs:90:1 91 | 92 LL | const BAD_UNINHABITED_WITH_DATA1: Result<(i32, Never), (i32, !)> = unsafe { mem::transmute(1u64) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of the never type `!` at .<enum-variant(Err)>.0.1 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of uninhabited type Never at .<enum-variant(Ok)>.0.1 94 | 95 = 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. 96 98 --> $DIR/ub-enum.rs:92:1 99 | 100 LL | const BAD_UNINHABITED_WITH_DATA2: Result<(i32, !), (i32, Never)> = unsafe { mem::transmute(1u64) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of uninhabited type Never at .<enum-variant(Err)>.0.1 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of the never type `!` at .<enum-variant(Ok)>.0.1 102 | 103 = 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. 104 The actual stderr differed from the expected stderr.

I just checked for endianness -- it failed the same way on powerpc64, but passed on powerpc64le and the rest of our little-endian targets.

Yeah this makes sense, the 1 in the u64 moves to the other end of the 8-byte range and thus doesn't land on the tag any more.

I think it is fine to use 0 instead of 1 as the constant here, which should make this endianess-independent.

I will prepare a PR to change this to 0.

rust-highfive assigned eddyb Mar 2, 2020

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

This comment has been minimized.

Sign in to view

eddyb reviewed Mar 2, 2020

View reviewed changes

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

eddyb reviewed Mar 2, 2020

View reviewed changes

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

eddyb reviewed Mar 2, 2020

View reviewed changes

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

eddyb reviewed Mar 2, 2020

View reviewed changes

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

eddyb approved these changes Mar 2, 2020

View reviewed changes

RalfJung commented Mar 2, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

eddyb approved these changes Mar 2, 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 Mar 2, 2020

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 4, 2020

RalfJung force-pushed the layout-visitor branch from e4ae6e6 to bd4c9a1 Compare March 4, 2020 08:12

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 4, 2020

RalfJung added 5 commits March 6, 2020 22:41

'fieldless enums' is not what I meant -- it's empty/uninhabited enums…

6548be2

…, really

miri value visitor: detect primitives by type, not layout

d47196b

const validation ub tests: use transmute instead of unions

aa1435b

test that we validate boxes

4807e93

test some more kinds of enums with uninhabited variants

f481547

Centril mentioned this pull request Mar 8, 2020

Rollup of 8 pull requests #69819

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 8, 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 8, 2020

This comment has been minimized.

Sign in to view

fix type size mismatch on 32bit

a95f00f

RalfJung force-pushed the layout-visitor branch from e623ea5 to a95f00f Compare March 8, 2020 11:42

Centril mentioned this pull request Mar 8, 2020

Rollup of 8 pull requests #69827

Closed

Centril mentioned this pull request Mar 8, 2020

Rollup of 7 pull requests #69829

Merged

bors merged commit c31b704 into rust-lang:master Mar 8, 2020

RalfJung deleted the layout-visitor branch March 8, 2020 21:05

cuviper reviewed Jul 1, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miri visitor: detect primitive types based on type, not layout (also, more tests) #69646

Miri visitor: detect primitive types based on type, not layout (also, more tests) #69646

RalfJung commented Mar 2, 2020

This comment has been minimized.

eddyb left a comment

RalfJung Mar 2, 2020

eddyb Mar 2, 2020

Zoxc Mar 2, 2020

RalfJung Mar 2, 2020

eddyb Mar 2, 2020

RalfJung Mar 2, 2020

RalfJung commented Mar 2, 2020

This comment has been minimized.

eddyb commented Mar 2, 2020

bors commented Mar 2, 2020

bors commented Mar 4, 2020

RalfJung commented Mar 4, 2020

bors commented Mar 4, 2020

Centril commented Mar 8, 2020

RalfJung commented Mar 8, 2020

RalfJung commented Mar 8, 2020

bors commented Mar 8, 2020

bors commented Mar 8, 2020

This comment has been minimized.

RalfJung commented Mar 8, 2020

bors commented Mar 8, 2020

bors commented Mar 8, 2020

cuviper Jul 1, 2020

RalfJung Jul 2, 2020

cuviper Jul 2, 2020

RalfJung Jul 3, 2020

RalfJung Jul 3, 2020

Miri visitor: detect primitive types based on type, not layout (also, more tests) #69646

Miri visitor: detect primitive types based on type, not layout (also, more tests) #69646

Conversation

RalfJung commented Mar 2, 2020

This comment has been minimized.

eddyb left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Mar 2, 2020

This comment has been minimized.

eddyb commented Mar 2, 2020

bors commented Mar 2, 2020

bors commented Mar 4, 2020

RalfJung commented Mar 4, 2020

bors commented Mar 4, 2020

Centril commented Mar 8, 2020

RalfJung commented Mar 8, 2020

RalfJung commented Mar 8, 2020

bors commented Mar 8, 2020

bors commented Mar 8, 2020

This comment has been minimized.

RalfJung commented Mar 8, 2020

bors commented Mar 8, 2020

bors commented Mar 8, 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