Draft: Miri/CTFE engine: Unions are not scalar #94411

RalfJung · 2022-02-27T01:53:18Z

To fix #69488 and by extension #94371, we should stop treating types like MaybeUninit<usize> as something that the Scalar type in the interpreter engine can represent. So we add a new test type_is_scalar which checks if a type can be represented as a Scalar.

This is a draft since it still ICEs in Miri, but I need help figuring out the best way to avoid that.

r? @oli-obk

rust-highfive · 2022-02-27T01:53:21Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

RalfJung · 2022-02-27T01:56:03Z

compiler/rustc_const_eval/src/interpret/operand.rs

+            ty.tuple_fields().iter().all(|field| type_is_scalar(tcx, field))
+        }
+        // FIXME: can these ever have Scalar ABI?
+        ty::Closure(..) | ty::Generator(..) => false,


This line is the problem: some closures (and likely generators) can end up with a Scalar layout, and they can even have niches, which then means validity will call read_scalar and cause an ICE when we do not treat it as a scalar.

To fix this, we need a way to iterate over the types of all the fields of (all the variants of) a closure/generator. Any suggestion for how to do that? :D I browsed the code for a bit and found nothing.

For closures you'll want https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/sty/struct.ClosureSubsts.html#method.upvar_tys which you get by copying the substs into a new instance of ClosureSubsts. The same helper exists for generators. I guess we should make this less cumbersome to do

What about renaming this function to type_can_be_scalar and returning true in all cases except unions?

Then we'd still have the bug for (e.g.) closures that capture unions in a way that they still get abi::Scalar layout.

I am experimenting with changing the layout to support undef scalars. If perf is happy with it, we can just ask the layout whether the scalars are undef and not do an immediate representation then.

oli-obk

I'm not too happy with this approach. Maybe we should add a flag to scalar layout information stating whether it has a union inside? Maybe other backends are interested in whether a scalar can be undef, too?

oli-obk · 2022-02-27T06:48:41Z

compiler/rustc_const_eval/src/interpret/operand.rs

+        // FIXME: can these ever have Scalar ABI?
+        ty::Closure(..) | ty::Generator(..) => false,
+        // Types that don't have scalar layout to begin with.
+        ty::Array(..) | ty::Slice(..) | ty::Str | ty::Dynamic(..) | ty::Foreign(..) => {


Single element arrays can have scalar layout, I think?

Two element arrays can be scalar pair, too, by the same logic, but maybe we don't do this?

At least currently, arrays always seem to have Aggregate ABI.

oli-obk · 2022-02-27T07:10:16Z

compiler/rustc_const_eval/src/interpret/operand.rs

+            ty.tuple_fields().iter().all(|field| type_is_scalar(tcx, field))
+        }
+        // FIXME: can these ever have Scalar ABI?
+        ty::Closure(..) | ty::Generator(..) => false,


For closures you'll want https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/sty/struct.ClosureSubsts.html#method.upvar_tys which you get by copying the substs into a new instance of ClosureSubsts. The same helper exists for generators. I guess we should make this less cumbersome to do

RalfJung · 2022-02-27T14:59:57Z

I'm not too happy with this approach. Maybe we should add a flag to scalar layout information stating whether it has a union inside? Maybe other backends are interested in whether a scalar can be undef, too?

I agree looking at the types is somewhat icky, and ideally abi::Scalar would contain the needed information. But that code is quite outside what I know how to handle...

FWIW, #93670 actually has some overlap with this I think. There, a new attribute was added to ArgAttributes -- but that only helps for function calls. Also in Miri we want to treat way more things as noundef (in particular, integers).

And indeed I think LLVM might some day be interested in this distinction -- i64 cannot be partially poison, so if Rust code can ever generate poison values (I think currently it cannot), then MaybeUninit<u64> cannot be compiled to just LLVM i64 any more, it would have to be something involving the "byte type" that was proposed to be added to LLVM.

RalfJung · 2022-02-27T15:02:55Z

For closures you'll want https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/sty/struct.ClosureSubsts.html#method.upvar_tys which you get by copying the substs into a new instance of ClosureSubsts. The same helper exists for generators. I guess we should make this less cumbersome to do

(I cannot reply directly to that comment, WTF GitHub?)

I was trying to figure out how to construct a ClosureSubst, but ClosureSubst::new did not help. Should I just construct a struct directly? But then I would be entirely ignoring the DefId in ty::Closure, which seems rather suspicious? At that point I decided this is probably wrong and gave up.

rust-log-analyzer · 2022-02-27T15:09:38Z

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Some tests failed in compiletest suite=mir-opt mode=mir-opt host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu

---- [mir-opt] mir-opt/const_prop/invalid_constant.rs stdout ----
24   
25       bb0: {
26           StorageLive(_1);                 // scope 0 at $DIR/invalid_constant.rs:21:9: 21:22
- -         _1 = const { InvalidChar { int: 0x110001 } }; // scope 0 at $DIR/invalid_constant.rs:21:25: 21:64
- +         _1 = const InvalidChar { int: 1114113_u32, chr: {transmute(0x00110001): char} }; // scope 0 at $DIR/invalid_constant.rs:21:25: 21:64
+           _1 = const { InvalidChar { int: 0x110001 } }; // scope 0 at $DIR/invalid_constant.rs:21:25: 21:64
29                                            // mir::Constant
30                                            // + span: $DIR/invalid_constant.rs:21:25: 21:64
- -                                          // + literal: Const { ty: InvalidChar, val: Unevaluated(main::{constant#0}, [main::InvalidChar], None) }
- +                                          // + literal: Const { ty: InvalidChar, val: Value(Scalar(0x00110001)) }
+                                            // + literal: Const { ty: InvalidChar, val: Unevaluated(main::{constant#0}, [main::InvalidChar], None) }
33           StorageLive(_2);                 // scope 1 at $DIR/invalid_constant.rs:28:9: 28:21
34           StorageLive(_3);                 // scope 1 at $DIR/invalid_constant.rs:28:25: 28:46
35           (_3.0: u32) = const 4_u32;       // scope 1 at $DIR/invalid_constant.rs:28:25: 28:46

- -         _2 = [move _3];                  // scope 1 at $DIR/invalid_constant.rs:28:24: 28:47
- +         _2 = [const InvalidTag { int: 4_u32, e: Scalar(0x00000004): E }]; // scope 1 at $DIR/invalid_constant.rs:28:24: 28:47
- +                                          // mir::Constant
- +                                          // + span: $DIR/invalid_constant.rs:28:24: 28:47
- +                                          // + literal: Const { ty: InvalidTag, val: Value(Scalar(0x00000004)) }
+           _2 = [move _3];                  // scope 1 at $DIR/invalid_constant.rs:28:24: 28:47
41           StorageDead(_3);                 // scope 1 at $DIR/invalid_constant.rs:28:46: 28:47
42           StorageLive(_4);                 // scope 2 at $DIR/invalid_constant.rs:35:9: 35:31
43           StorageLive(_5);                 // scope 2 at $DIR/invalid_constant.rs:35:35: 35:56

44           (_5.0: u32) = const 0_u32;       // scope 2 at $DIR/invalid_constant.rs:35:35: 35:56
- -         _4 = [move _5];                  // scope 2 at $DIR/invalid_constant.rs:35:34: 35:57
- +         _4 = [const NoVariants { int: 0_u32, empty: Scalar(<ZST>): Empty }]; // scope 2 at $DIR/invalid_constant.rs:35:34: 35:57
- +                                          // mir::Constant
- +                                          // + span: $DIR/invalid_constant.rs:35:34: 35:57
- +                                          // + literal: Const { ty: NoVariants, val: Value(Scalar(0x00000000)) }
+           _4 = [move _5];                  // scope 2 at $DIR/invalid_constant.rs:35:34: 35:57
50           StorageDead(_5);                 // scope 2 at $DIR/invalid_constant.rs:35:56: 35:57
51           StorageLive(_6);                 // scope 3 at $DIR/invalid_constant.rs:39:9: 39:22
52           nop;                             // scope 0 at $DIR/invalid_constant.rs:15:11: 42:2

thread '[mir-opt] mir-opt/const_prop/invalid_constant.rs' panicked at 'Actual MIR output differs from expected MIR output /checkout/src/test/mir-opt/const_prop/invalid_constant.main.ConstProp.diff', src/tools/compiletest/src/runtest.rs:3386:25


failures:
    [mir-opt] mir-opt/const_prop/invalid_constant.rs

oli-obk · 2022-02-27T15:17:47Z

I can check on Tuesday, but my approach is basically "check other use sites, do the same thing and document the findings and/or add helpers"

RalfJung · 2022-03-09T20:47:36Z

Closing in favor of #94527.

…esjardin Let CTFE to handle partially uninitialized unions without marking the entire value as uninitialized. follow up to rust-lang#94411 To fix rust-lang#69488 and by extension fix rust-lang#94371, we should stop treating types like `MaybeUninit<usize>` as something that the `Scalar` type in the interpreter engine can represent. So we add a new field to `abi::Primitive` that records whether the primitive is nested in a union cc `@RalfJung` r? `@ghost`

Miri/CTFE engine: do not treat unions as scalar values

351dfa2

rust-highfive assigned oli-obk Feb 27, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 27, 2022

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

RalfJung commented Feb 27, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung mentioned this pull request Feb 27, 2022

mem::swap behaves incorrectly in CTFE (and Miri) #94371

Closed

oli-obk reviewed Feb 27, 2022

View reviewed changes

RalfJung mentioned this pull request Feb 27, 2022

For MIRI, cfg out the swap vectorization logic from 94212 #94412

Merged

add tests

ed7ca44

RalfJung force-pushed the unions-are-not-scalar branch from 45543cc to ed7ca44 Compare February 27, 2022 14:56

RalfJung mentioned this pull request Feb 27, 2022

const-eval: load of partially initialized scalar produces entirely uninitialized result #69488

Closed

oli-obk mentioned this pull request Mar 2, 2022

Let CTFE to handle partially uninitialized unions without marking the entire value as uninitialized. #94527

Merged

RalfJung closed this Mar 9, 2022

RalfJung deleted the unions-are-not-scalar branch October 29, 2022 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Miri/CTFE engine: Unions are not scalar #94411

Draft: Miri/CTFE engine: Unions are not scalar #94411

RalfJung commented Feb 27, 2022

rust-highfive commented Feb 27, 2022

RalfJung Feb 27, 2022

oli-obk Feb 27, 2022

bjorn3 Mar 1, 2022 •

edited

Loading

RalfJung Mar 1, 2022

oli-obk Mar 1, 2022

This comment has been minimized.

oli-obk left a comment

oli-obk Feb 27, 2022

oli-obk Feb 27, 2022

RalfJung Feb 27, 2022

oli-obk Feb 27, 2022

RalfJung commented Feb 27, 2022 •

edited

Loading

RalfJung commented Feb 27, 2022

rust-log-analyzer commented Feb 27, 2022

oli-obk commented Feb 27, 2022

RalfJung commented Mar 9, 2022

Draft: Miri/CTFE engine: Unions are not scalar #94411

Draft: Miri/CTFE engine: Unions are not scalar #94411

Conversation

RalfJung commented Feb 27, 2022

rust-highfive commented Feb 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn3 Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

oli-obk 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

RalfJung commented Feb 27, 2022 • edited Loading

RalfJung commented Feb 27, 2022

rust-log-analyzer commented Feb 27, 2022

oli-obk commented Feb 27, 2022

RalfJung commented Mar 9, 2022

bjorn3 Mar 1, 2022 •

edited

Loading

RalfJung commented Feb 27, 2022 •

edited

Loading