tighten sanity checks around Scalar and ScalarPair #96220

RalfJung · 2022-04-19T18:19:19Z

While investigating #96185 I noticed codegen has tighter sanity checks here than Miri does, so I added some more assertions. Strangely, some of them fail, so I also needed to add a HACK... that is probably worth looking into.

This does not fix that issue, but it changes the ICE messages, making it quite clear that we have a scalar whose size is not the same as that of the surrounding layout.

r? @oli-obk

rust-highfive · 2022-04-19T18:19:22Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

RalfJung · 2022-04-19T18:20:13Z

compiler/rustc_const_eval/src/interpret/place.rs

-                    // FIXME: Can we check anything here?
-                }
-            }
-        }


I removed all this since I think it is redundant with the scalar size check in to_bits that alloc.write_scalar does.

RalfJung · 2022-04-19T18:32:06Z

compiler/rustc_const_eval/src/interpret/operand.rs

+            // HACK to support buggy enums whose variants have Aggregate ABI
+            // (see https://github.com/rust-lang/rust/issues/96185).
+            // codegen doesn't do this, not sure why.
+            (Immediate::ScalarPair(a_val, b_val), Abi::Aggregate { .. }) => {


This is the odd one -- a ScalarPair at Aggregate ABI just doesn't seem right.

I added sanity checking in operand_downcast, and my theory was confirmed: when downcasting std::option::Option<*mut std::ffi::c_void> to variant 1, the ABI changes from ScalarPair to Aggegreate.

downcast to 1 turned ScalarPair layout into non-scalar layout: TyAndLayout { ty: std::option::Option<*mut std::ffi::c_void>, layout: Layout { fields: Arbitrary { offsets: [ Size { raw: 0, }, ], memory_index: [ 0, ], }, variants: Multiple { tag: Initialized { value: Int( I64, false, ), valid_range: 0..=1, }, tag_encoding: Direct, tag_field: 0, variants: [ Layout { fields: Arbitrary { offsets: [], memory_index: [], }, variants: Single { index: 0, }, abi: Aggregate { sized: true, }, largest_niche: None, align: AbiAndPrefAlign { abi: Align { pow2: 0, }, pref: Align { pow2: 3, }, }, size: Size { raw: 8, }, }, Layout { fields: Arbitrary { offsets: [ Size { raw: 8, }, ], memory_index: [ 0, ], }, variants: Single { index: 1, }, abi: Aggregate { sized: true, }, largest_niche: None, align: AbiAndPrefAlign { abi: Align { pow2: 3, }, pref: Align { pow2: 3, }, }, size: Size { raw: 16, }, }, ], }, abi: ScalarPair( Initialized { value: Int( I64, false, ), valid_range: 0..=1, }, Initialized { value: Pointer, valid_range: 0..=18446744073709551615, }, ), largest_niche: Some( Niche { offset: Size { raw: 0, }, value: Int( I64, false, ), valid_range: 0..=1, }, ), align: AbiAndPrefAlign { abi: Align { pow2: 3, }, pref: Align { pow2: 3, }, }, size: Size { raw: 16, }, }, } to TyAndLayout { ty: std::option::Option<*mut std::ffi::c_void>, layout: Layout { fields: Arbitrary { offsets: [ Size { raw: 8, }, ], memory_index: [ 0, ], }, variants: Single { index: 1, }, abi: Aggregate { sized: true, }, largest_niche: None, align: AbiAndPrefAlign { abi: Align { pow2: 3, }, pref: Align { pow2: 3, }, }, size: Size { raw: 16, }, }, }

Reported as #96221.

RalfJung · 2022-04-19T21:55:53Z

Ah, I guess this is enough to anger const-eval on the runtime version of this example.^^

I guess this is blocked on fixing that enum layout then.

oli-obk · 2022-04-20T09:15:49Z

Const prop strikes again 🙈

RalfJung · 2022-04-20T12:43:34Z

The curious part is that const prop managed to dodge the assertions we had in place before. I guess that goes to show some extra assertions are really needed. ;)

bors · 2022-05-05T19:40:33Z

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

RalfJung · 2022-05-10T12:28:53Z

I rebased and commented out the things that are broken by #96185. @oli-obk I think this is ready for review.

Also let's ensure this does not cost too much perf.
@bors try @rust-timer queue

rust-timer · 2022-05-10T12:28:55Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-05-10T12:29:01Z

⌛ Trying commit aef8a93 with merge e20aa27db438b844d57419af48c40860d88dbcda...

rust-timer · 2022-05-11T12:32:01Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-05-11T12:32:08Z

⌛ Trying commit 14f6daf with merge bae4e5b81419bd94533a9428d118973194da792b...

bors · 2022-05-11T13:55:02Z

☀️ Try build successful - checks-actions
Build commit: bae4e5b81419bd94533a9428d118973194da792b (bae4e5b81419bd94533a9428d118973194da792b)

rust-timer · 2022-05-11T13:55:04Z

Queued bae4e5b81419bd94533a9428d118973194da792b with parent f296b9a, future comparison URL.

rust-timer · 2022-05-11T15:27:40Z

Finished benchmarking commit (bae4e5b81419bd94533a9428d118973194da792b): comparison url.

Summary:

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regressions found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	0	3	0	0	0
mean²	N/A	1.4%	N/A	N/A	N/A
max	N/A	1.5%	N/A	N/A	N/A

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

number of relevant changes ↩
the arithmetic mean of the percent change ↩

RalfJung · 2022-05-11T15:31:13Z

Hm, well, that did not help...

Maybe the match on a pair (*base, base.layout.abi) compiles less nicely than the previous code?
All but one arm of this match is _, so I could probably rewrite it in different style. Not sure if that's worth it though, LLVM will probably realize that this is what the control flow boils down to.

oli-obk · 2022-05-11T15:53:07Z

mir interpret is always hitting LLVM inlining thresholds, I don't think it's worth looking into further in this PR

@bors r+

bors · 2022-05-11T15:53:09Z

📌 Commit 14f6daf has been approved by oli-obk

bors · 2022-05-11T16:29:23Z

⌛ Testing commit 14f6daf with merge 3e2c8514996fffe0081afd8cd6dcf2fcf362d4ff...

rust-log-analyzer · 2022-05-11T18:28:00Z

A job failed! Check out the build log: (web) (plain)

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

bors · 2022-05-11T18:28:29Z

💔 Test failed - checks-actions

oli-obk · 2022-05-11T18:32:33Z

@bors retry timeout, logs empty/stuck

bors · 2022-05-11T18:34:17Z

⌛ Testing commit 14f6daf with merge 6dd6840...

bors · 2022-05-11T21:14:55Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 6dd6840 to master...

rust-timer · 2022-05-11T23:44:57Z

Finished benchmarking commit (6dd6840): comparison url.

Summary:

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regressions found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	0	4	0	0	0
mean²	N/A	1.1%	N/A	N/A	N/A
max	N/A	1.5%	N/A	N/A	N/A

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

number of relevant changes ↩
the arithmetic mean of the percent change ↩

rust-highfive assigned oli-obk Apr 19, 2022

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

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

RalfJung commented Apr 19, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the scalar-no-padding branch from a9666f7 to 60c3d1a Compare April 19, 2022 18:27

RalfJung commented Apr 19, 2022

View reviewed changes

RalfJung force-pushed the scalar-no-padding branch from 60c3d1a to e7d1ace Compare April 19, 2022 18:34

This comment has been minimized.

Sign in to view

RalfJung force-pushed the scalar-no-padding branch from d448749 to 64826e5 Compare April 20, 2022 01:33

This comment has been minimized.

Sign in to view

oli-obk added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Apr 20, 2022

RalfJung force-pushed the scalar-no-padding branch from 64826e5 to fc4846f Compare May 10, 2022 11:14

tighten sanity checks around Scalar and ScalarPair

7196556

This comment has been minimized.

Sign in to view

RalfJung added 3 commits May 10, 2022 14:23

disable one check for now until rust-lang#96185 is fixed

79c169d

even tighter checks for layouts on immediate field projections

600d960

update/remove some old comments

aef8a93

RalfJung force-pushed the scalar-no-padding branch from fc4846f to aef8a93 Compare May 10, 2022 12:26

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 10, 2022

RalfJung removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label May 10, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 11, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 11, 2022

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 11, 2022

bors mentioned this pull request May 11, 2022

Gracefully fail to resolve associated items instead of delay_span_bug. #96806

Merged

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

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 11, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label May 11, 2022

bors merged commit 6dd6840 into rust-lang:master May 11, 2022

rustbot added this to the 1.62.0 milestone May 11, 2022

RalfJung deleted the scalar-no-padding branch May 12, 2022 07:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tighten sanity checks around Scalar and ScalarPair #96220

tighten sanity checks around Scalar and ScalarPair #96220

RalfJung commented Apr 19, 2022 •

edited

Loading

rust-highfive commented Apr 19, 2022

RalfJung Apr 19, 2022

This comment has been minimized.

RalfJung Apr 19, 2022

RalfJung Apr 19, 2022

This comment has been minimized.

RalfJung commented Apr 19, 2022

This comment has been minimized.

oli-obk commented Apr 20, 2022

RalfJung commented Apr 20, 2022

bors commented May 5, 2022

This comment has been minimized.

RalfJung commented May 10, 2022

rust-timer commented May 10, 2022

bors commented May 10, 2022

rust-timer commented May 11, 2022

bors commented May 11, 2022

bors commented May 11, 2022

rust-timer commented May 11, 2022

rust-timer commented May 11, 2022

RalfJung commented May 11, 2022

oli-obk commented May 11, 2022

bors commented May 11, 2022

bors commented May 11, 2022

rust-log-analyzer commented May 11, 2022

bors commented May 11, 2022

oli-obk commented May 11, 2022

bors commented May 11, 2022

bors commented May 11, 2022

rust-timer commented May 11, 2022

tighten sanity checks around Scalar and ScalarPair #96220

tighten sanity checks around Scalar and ScalarPair #96220

Conversation

RalfJung commented Apr 19, 2022 • edited Loading

rust-highfive commented Apr 19, 2022

RalfJung Apr 19, 2022

Choose a reason for hiding this comment

This comment has been minimized.

RalfJung Apr 19, 2022

Choose a reason for hiding this comment

RalfJung Apr 19, 2022

Choose a reason for hiding this comment

This comment has been minimized.

RalfJung commented Apr 19, 2022

This comment has been minimized.

oli-obk commented Apr 20, 2022

RalfJung commented Apr 20, 2022

bors commented May 5, 2022

This comment has been minimized.

RalfJung commented May 10, 2022

rust-timer commented May 10, 2022

bors commented May 10, 2022

rust-timer commented May 11, 2022

bors commented May 11, 2022

bors commented May 11, 2022

rust-timer commented May 11, 2022

rust-timer commented May 11, 2022

Footnotes

RalfJung commented May 11, 2022

oli-obk commented May 11, 2022

bors commented May 11, 2022

bors commented May 11, 2022

rust-log-analyzer commented May 11, 2022

bors commented May 11, 2022

oli-obk commented May 11, 2022

bors commented May 11, 2022

bors commented May 11, 2022

rust-timer commented May 11, 2022

Footnotes

RalfJung commented Apr 19, 2022 •

edited

Loading