Always error on `SizeOverflow` during mir evaluation #63152

estebank · 2019-07-31T00:47:31Z

Centril · 2019-07-31T02:01:21Z

src/test/ui/consts/issue-55878.stderr

+LL |     println!("Size: {}", std::mem::size_of::<[u8; std::u64::MAX as usize]>());
+   |                          ---------------------------------------------------
+   |
+   = note: `#[deny(const_err)]` on by default


Wait... the issue describes the snippet above as a soundness hole enabling illegal instructions yet this is using a lint to ensure soundness?

yea, we'd need to change

rust/src/librustc_mir/const_eval.rs

Line 687 in d1d79ae

if let EvalErrorKind::ReferencedConstant = err.error {

, too

but all of this is just papering over the issue. I think you can also ICE the compiler if you do this size_of call inside a static.

Note that there is no unsoundness. I'm certain that we are hitting

rust/src/librustc_codegen_ssa/mir/operand.rs

Lines 480 to 489 in a7f2867

// Allow RalfJ to sleep soundly knowing that even refactorings that remove

// the above error (or silence it under some conditions) will not cause UB

bx.abort();

// We've errored, so we don't have to produce working code.

let layout = bx.cx().layout_of(ty);

bx.load_operand(PlaceRef::new_sized(

bx.cx().const_undef(bx.cx().type_ptr_to(bx.cx().backend_type(layout))),

layout,

layout.align.abi,

))

We need to redesign this properly.

@estebank what were the tests that broke when you inserted the unconditional delay_span_bug?

During compilation I got a lot of the following

error: internal compiler error: erroneous constant used --> src/libcore/mem/mod.rs:243:5 | 243 | intrinsics::size_of::<T>() | ^^^^^^^^^^^^^^^^^^^^^^^^ error: internal compiler error: erroneous constant used --> src/libcore/num/mod.rs:171:16 | 171 | self.0.fmt(f) | ^^^ error: internal compiler error: erroneous constant used --> src/libcore/num/dec2flt/algorithm.rs:211:25 | 211 | z = prev_float(z); | ^^^^^^^^^^

@Centril turned it into an error

Note that there is no unsoundness. I'm certain that we are hitting

/me is happy that a block of code commented "Allow RalfJ to sleep soundly" is preventing this from being a soundness issue. ;)

src/librustc/mir/interpret/error.rs

estebank · 2019-08-03T23:00:11Z

While I was at it I took a detour to also fix #25116 and give "type too big" errors a span.

src/librustc_codegen_ssa/mir/analyze.rs

src/librustc_codegen_ssa/mir/rvalue.rs

oli-obk · 2019-08-04T07:37:10Z

src/test/ui/consts/issue-55878.stderr

+  --> $SRC_DIR/libcore/mem/mod.rs:LL:COL
+   |
+LL |     intrinsics::size_of::<T>()
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^ the type `[u8; 18446744073709551615]` is too big for the current architecture


I'm fine merging as is with a FIXME in place for adding a useful (not duplicated) note here. It's likely a bit involeved in the const eval engine, since the way I'm imagining it requires annotating every layout_of call.

Yeah, I thought about that too and came to the same conclusion.

I think I have a way to make a separate label that won't be too hacky. What do you think a good wording for this error would be/what the end result should look like?

The problem is that there are many situations in const eval that compute layouts and could thus fail. I think we should label each with their own message. If we hardcode a message for just this case it will be nonsensical in all other cases.

I just removed the label altogether and left the main message only

oli-obk · 2019-08-04T07:44:59Z

src/test/ui/huge-array-simple.stderr

@@ -1,4 +1,8 @@
 error: the type `[u8; N]` is too big for the current architecture
+  --> $DIR/huge-array-simple.rs:12:9
+   |
+LL |     let _fat : [u8; (1<<61)+(1<<31)] =


That span isn't ideal, but it's hard to do better in the MIR, especially as there may be no HIR local in the case of temporaries. Not sure what to do about it

I agree, but at the same time it's a huge improvement over the current "good luck finding where the problem is" state of this error.

src/test/ui/huge-enum.rs

oli-obk · 2019-08-04T20:07:58Z

r=me with 32 bit test fixed

estebank · 2019-08-04T23:30:13Z

@bors r=oli-obk

bors · 2019-08-04T23:30:14Z

📌 Commit f621f89 has been approved by oli-obk

@oli-obk

Always error on `SizeOverflow` during mir evaluation Fix rust-lang#55878, fix rust-lang#25116. r? @oli-obk

Centril · 2019-08-06T13:35:36Z

@bors r- Failed in #63325 (comment).

estebank · 2019-08-06T17:30:10Z

@bors try

bors · 2019-08-06T17:30:23Z

⌛ Trying commit 613c0a8 with merge fd57268...

@oli-obk

Always error on `SizeOverflow` during mir evaluation Fix #55878, fix #25116. r? @oli-obk

estebank · 2019-08-06T17:31:19Z

@Centril will try test all targets?

I moved the failing ui test to compile-fail, as I don't see how to resolve the discrepancy otherwise.

src/test/ui/huge-array-simple-64.rs

src/test/ui/issues/issue-15919-64.stderr

RalfJung · 2019-08-06T17:48:41Z

will try test all targets?

No. It doesn't really test much more than what is tested in a PR already; the reason to do a try build is if you want a built compiler to download and test locally, or do perf/crater.

RalfJung · 2019-08-06T17:51:24Z

src/test/compile-fail/consts/issue-55878.rs

+// error-pattern: is too big for the current architecture
+fn main() {
+    println!("Size: {}", std::mem::size_of::<[u8; std::u64::MAX as usize]>());
+}


What is the error here for 32bit vs 64bit, that this cannot be handled by normalization?

Missing a span from (at least) one specific target that is present in others:

error[E0080]: the type `[u8; SIZE]` is too big for the current architecture --> $SRC_DIR/libcore/mem/mod.rs:LL:COL | LL | intrinsics::size_of::<T>() | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | ::: $DIR/issue-55878.rs:6:26 | LL | println!("Size: {}", std::mem::size_of::<[u8; std::u64::MAX as usize]>()); | ---------------------------------------------------

vs

error[E0080]: the type `[u8; SIZE]` is too big for the current architecture | ::: $DIR/issue-55878.rs:6:26 | LL | println!("Size: {}", std::mem::size_of::<[u8; std::u64::MAX as usize]>()); | ---------------------------------------------------

That's extremely odd, isn't it?

Normalization could still handle this -- or you use the two-test-cases approach with ignore-32bit and ignore-64bit? AFAIK compile-fail is on the way out.

The divergence is not due to 32bit vs 64bit, it's because the dist-i586-gnu-i586-i686-musl rustc seems to be packaged slightly different than the other targets, so we miss core and std information to point spans to. Adding this info was a relatively recent improvement from the last year.

I agree that compile-fail should be discouraged, but short of waiting for us to fix the packaging issue I don't see a good solution to this.

Oh I see.

Is it possible to ignore musl targets, or so?

It is but I have no way of knowing a priori wether this is a musl-only discrepancy or not.

I'm not super happy about adding more compile-fail tests, but oh well... :)

src/test/ui/issues/issue-15919-64.rs

src/test/ui/issues/issue-15919-32.rs

estebank · 2019-08-06T23:51:53Z

@bors r=oli-obk

crosses fingers

bors · 2019-08-06T23:51:54Z

📌 Commit 3144b0a has been approved by oli-obk

bors · 2019-08-07T04:28:15Z

⌛ Testing commit 3144b0a with merge 5421d94...

@oli-obk

Always error on `SizeOverflow` during mir evaluation Fix #55878, fix #25116. r? @oli-obk

bors · 2019-08-07T08:06:36Z

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing 5421d94 to master...

rust-highfive assigned oli-obk Jul 31, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 31, 2019

Centril reviewed Jul 31, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

estebank added 3 commits August 2, 2019 12:30

Always error on SizeOverflow during mir evaluation

83b5eb9

Do not lint on SizeOverflow, always error

58bd878

Nicer labels for type layout errors

d3da411

estebank force-pushed the big-array branch from abcb711 to d3da411 Compare August 2, 2019 19:34

oli-obk mentioned this pull request Aug 3, 2019

Making enums more specific in src/librustc/mir/interpret/error.rs rust-lang/const-eval#4

Closed

4 tasks

RalfJung reviewed Aug 3, 2019

View reviewed changes

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

oli-obk reviewed Aug 3, 2019

View reviewed changes

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

avoid mutable state and override main message

2c56842

This comment has been minimized.

Sign in to view

Point to local place span on "type too big" error

db099fb

estebank force-pushed the big-array branch from ffa3873 to db099fb Compare August 4, 2019 05:00

Simplify change to layout_of

2340067

oli-obk requested changes Aug 4, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

review comments: clean up

387dcff

estebank force-pushed the big-array branch from 4f39c8f to 387dcff Compare August 4, 2019 17:42

This comment has been minimized.

Sign in to view

tweak output and tests

bdd79b8

oli-obk reviewed Aug 4, 2019

View reviewed changes

src/test/ui/huge-enum.rs Outdated Show resolved Hide resolved

revert change to single test

f621f89

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 Aug 4, 2019

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 6, 2019

Centril added a commit to Centril/rust that referenced this pull request Aug 6, 2019

Rollup merge of rust-lang#63152 - estebank:big-array, r=oli-obk

44eab0c

Always error on `SizeOverflow` during mir evaluation Fix rust-lang#55878, fix rust-lang#25116. r? @oli-obk

Centril mentioned this pull request Aug 6, 2019

Rollup of 5 pull requests #63325

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 Aug 6, 2019

move error with diverging output to compile-fail

613c0a8

bors added a commit that referenced this pull request Aug 6, 2019

Auto merge of #63152 - estebank:big-array, r=<try>

fd57268

Always error on `SizeOverflow` during mir evaluation Fix #55878, fix #25116. r? @oli-obk