Custom MIR: Support cleanup blocks #117330

tmiasko · 2023-10-28T21:59:24Z

Cleanup blocks are declared with bb (cleanup) = { ... }.

Call and Drop terminators take an additional argument describing the unwind action, which is one of the following:

UnwindContinue()
UnwindUnreachable()
UnwindTerminate(reason), where reason is ReasonAbi or ReasonInCleanup
UnwindCleanup(block)

Also support unwind resume and unwind terminate terminators:

UnwindResume()
UnwindTerminate(reason)

rustbot · 2023-10-28T21:59:31Z

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-10-28T22:46:55Z

The Miri subtree was changed

cc @rust-lang/miri

library/core/src/intrinsics/mir.rs

RalfJung · 2023-10-29T07:59:59Z

library/core/src/intrinsics/mir.rs

 //!
 //!         {
-//!             Call(_unused = Vec::push(v, value), pop)
+//!             Call(_unused = Vec::push(v, value), pop, Continue())


It seems like most of the time we want Continue. So maybe with can have Call always use the default unwind action of Continue, and then have CallWithUnwind for when one wants something else? Or maybe some other way to have default values.

RalfJung · 2023-10-29T08:00:30Z

library/core/src/intrinsics/mir.rs

 //!
 //!         pop = {
-//!             Call(popped = Vec::pop(v), drop)
+//!             Call(popped = Vec::pop(v), drop, Continue())


This makes me wonder if we should replace Call(popped = Vec::pop(v), drop) by Call(popped = Vec::pop(v), ReturnTo(drop)) or something like that... basically emulating named arguments. That's orthogonal to your PR though so I'd also be happy to do it as a follow-up if you prefer.

library/backtrace

compiler/rustc_mir_build/src/build/custom/parse.rs

library/core/src/intrinsics/mir.rs

cjgillot · 2023-11-13T23:39:52Z

@bors r+

bors · 2023-11-13T23:39:55Z

📌 Commit 3d89c80 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-11-14T02:01:03Z

⌛ Testing commit 3d89c80 with merge 060f778...

…=cjgillot Custom MIR: Support cleanup blocks Cleanup blocks are declared with `bb (cleanup) = { ... }`. `Call` and `Drop` terminators take an additional argument describing the unwind action, which is one of the following: * `UnwindContinue()` * `UnwindUnreachable()` * `UnwindTerminate(reason)`, where reason is `ReasonAbi` or `ReasonInCleanup` * `UnwindCleanup(block)` Also support unwind resume and unwind terminate terminators: * `UnwindResume()` * `UnwindTerminate(reason)`

bors · 2023-11-14T02:22:58Z

💔 Test failed - checks-actions

Cleanup blocks are declared with `bb (cleanup) = { ... }`. `Call` and `Drop` terminators take an additional argument describing the unwind action, which is one of the following: * `UnwindContinue()` * `UnwindUnreachable()` * `UnwindTerminate(reason)`, where reason is `ReasonAbi` or `ReasonInCleanup` * `UnwindCleanup(block)` Also support unwind resume and unwind terminate terminators: * `UnwindResume()` * `UnwindTerminate(reason)`

tmiasko · 2023-11-14T07:30:01Z

Cannot UnwindResume in a function that cannot unwind

Added needs-unwind to the test.

By the way, it seems quite suspect that MIR validity depends on the current panic strategy, since it means that code generation operates on invalid MIR when it originates from a crate with a different panic strategy.

tmiasko · 2023-11-14T08:00:21Z

@bors r=cjgillot

bors · 2023-11-14T08:00:23Z

📌 Commit 78da577 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-11-14T08:53:28Z

⌛ Testing commit 78da577 with merge 5526682...

bors · 2023-11-14T10:54:33Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 5526682 to master...

rust-timer · 2023-11-14T12:17:56Z

Finished benchmarking commit (5526682): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-0.6%, -0.6%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.2%	[1.2%, 1.2%]	1
Regressions ❌ (secondary)	2.7%	[2.0%, 4.0%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.9%	[-3.9%, -2.4%]	3
All ❌✅ (primary)	1.2%	[1.2%, 1.2%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.1%	[2.0%, 4.2%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.3%, -1.9%]	3
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 672.988s -> 673.782s (0.12%)
Artifact size: 311.12 MiB -> 311.12 MiB (0.00%)

rustbot assigned cjgillot Oct 28, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 28, 2023

tmiasko force-pushed the custom-mir-cleanup-blocks branch from 73d4334 to bff1419 Compare October 28, 2023 22:00

This comment has been minimized.

Sign in to view

tmiasko force-pushed the custom-mir-cleanup-blocks branch from bff1419 to 9e68f0c Compare October 28, 2023 22:46

This comment has been minimized.

Sign in to view

tmiasko force-pushed the custom-mir-cleanup-blocks branch from 9e68f0c to f54f88f Compare October 29, 2023 06:16

RalfJung reviewed Oct 29, 2023

View reviewed changes

library/core/src/intrinsics/mir.rs Outdated Show resolved Hide resolved

RalfJung reviewed Oct 29, 2023

View reviewed changes

tmiasko mentioned this pull request Oct 29, 2023

Fix def-use check for call terminators #117359

Merged

tmiasko force-pushed the custom-mir-cleanup-blocks branch 2 times, most recently from 1c827a9 to 73ff6c9 Compare October 30, 2023 09:23

This comment has been minimized.

Sign in to view

tmiasko force-pushed the custom-mir-cleanup-blocks branch from 73ff6c9 to d58e6d9 Compare October 30, 2023 09:27

This comment has been minimized.

Sign in to view

tmiasko force-pushed the custom-mir-cleanup-blocks branch from d58e6d9 to 2f0d557 Compare October 30, 2023 09:48

cjgillot reviewed Oct 31, 2023

View reviewed changes

library/backtrace Show resolved Hide resolved

compiler/rustc_mir_build/src/build/custom/parse.rs Outdated Show resolved Hide resolved

compiler/rustc_mir_build/src/build/custom/parse.rs Outdated Show resolved Hide resolved

library/core/src/intrinsics/mir.rs Show resolved Hide resolved

cjgillot 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2023

tmiasko force-pushed the custom-mir-cleanup-blocks branch from 2f0d557 to fc64498 Compare November 9, 2023 13:52

tmiasko added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 9, 2023

tmiasko force-pushed the custom-mir-cleanup-blocks branch from fc64498 to efe98b3 Compare November 9, 2023 14:37

RalfJung reviewed Nov 9, 2023

View reviewed changes

library/core/src/intrinsics/mir.rs Outdated Show resolved Hide resolved

tmiasko force-pushed the custom-mir-cleanup-blocks branch 2 times, most recently from e059888 to 3d89c80 Compare November 9, 2023 15:52

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 Nov 13, 2023

This comment has been minimized.

Sign in to view

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 Nov 14, 2023

tmiasko force-pushed the custom-mir-cleanup-blocks branch from 3d89c80 to 78da577 Compare November 14, 2023 07:25

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 Nov 14, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 14, 2023

bors merged commit 5526682 into rust-lang:master Nov 14, 2023

rustbot added this to the 1.76.0 milestone Nov 14, 2023

tmiasko deleted the custom-mir-cleanup-blocks branch November 14, 2023 10:55

Uh oh!

Custom MIR: Support cleanup blocks #117330

Custom MIR: Support cleanup blocks #117330

Uh oh!

Conversation

tmiasko commented Oct 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 28, 2023

Uh oh!

This comment has been minimized.

rustbot commented Oct 28, 2023

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

RalfJung Oct 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Oct 29, 2023

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cjgillot commented Nov 13, 2023

Uh oh!

bors commented Nov 13, 2023

Uh oh!

bors commented Nov 14, 2023

Uh oh!

This comment has been minimized.

bors commented Nov 14, 2023

Uh oh!

tmiasko commented Nov 14, 2023

Uh oh!

tmiasko commented Nov 14, 2023

Uh oh!

bors commented Nov 14, 2023

Uh oh!

bors commented Nov 14, 2023

Uh oh!

bors commented Nov 14, 2023

Uh oh!

rust-timer commented Nov 14, 2023

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tmiasko commented Oct 28, 2023 •

edited

Loading

RalfJung Oct 29, 2023 •

edited

Loading