Optimize catch_unwind to match C++ try/catch by Mark-Simulacrum · Pull Request #67502 · rust-lang/rust

Mark-Simulacrum · 2019-12-22T00:48:08Z

This refactors the implementation of catching unwinds to allow LLVM to inline the "try" closure directly into the happy path, avoiding indirection. This means that the catch_unwind implementation is (after this PR) zero-cost unless a panic is thrown.

https://rust.godbolt.org/z/cZcUSB is an example of the current codegen in a simple case. Notably, the codegen is exactly the same if -Cpanic=abort is passed, which is clearly not great.

This PR, on the other hand, generates the following assembly:

# -Cpanic=unwind:
	push   rbx
	mov    ebx,0x2a
	call   QWORD PTR [rip+0x1c53c]        # <happy>
	mov    eax,ebx
	pop    rbx
	ret
	mov    rdi,rax
	call   QWORD PTR [rip+0x1c537]        # cleanup function call
	call   QWORD PTR [rip+0x1c539]        # <unfortunate>
	mov    ebx,0xd
	mov    eax,ebx
	pop    rbx
	ret

# -Cpanic=abort:
	push   rax
	call   QWORD PTR [rip+0x20a1]        # <happy>
	mov    eax,0x2a
	pop    rcx
	ret

Fixes #64224, and resolves #64222.

rust-highfive · 2019-12-22T00:48:12Z

r? @shepmaster

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

Mark-Simulacrum · 2019-12-22T00:49:26Z

I suspect that the best reviewer here is r? @alexcrichton.

I am not sure whether we want to accept this. I created this patch while trying to investigate #64224; it is a slight simplification of the generated assembly, but not really anything too major (as might be expected).

src/libpanic_unwind/emcc.rs

src/libstd/panicking.rs

src/libpanic_unwind/lib.rs

src/libstd/panicking.rs

Mark-Simulacrum · 2019-12-27T18:40:37Z

I believe the test failure here is because we treat -Zno-landing-pads and -Cpanic=abort equivalently today, which has worked so far because -Zno-landing-pads was never passed to the libpanic_unwind compiler. These two options mean different things though: -Zno-landing-pads means that a panic will not run destructors, but will still unwind, whereas -Cpanic=abort will just immediately abort.

I'm not actually sure if there's much use to -Zno-landing-pads -- is there a situation where you want to unwind but not actually run destructors and so forth? I think the answer is no, or at least, it doesn't make much sense to me. I would suggest that we drop that flag entirely.

That would mean that the test in question would also be deleted; we do not expect that a crate with -Cpanic=abort can catch panics (even at thread boundaries).

Mark-Simulacrum · 2019-12-28T13:56:53Z

@bors try for easier testing

bors · 2019-12-28T13:57:04Z

⌛ Trying commit ba58f6421d4c5598f16479b0b2e5a1357917bac0 with merge d19982ed64d76225c52b773a53d593e6e96282e8...

bors · 2019-12-28T16:47:10Z

☀️ Try build successful - checks-azure
Build commit: d19982ed64d76225c52b773a53d593e6e96282e8 (d19982ed64d76225c52b773a53d593e6e96282e8)

Amanieu · 2019-12-29T23:28:55Z

Should be good to merge at this point. I moved some unrelated commits fixing some memory leaks to a separate PR (#67711).

alexcrichton

One thing I'm worried about here is that this PR changes the standard library to unconditionally use the try intrinsic no matter what panic mode is selected. Previously the try intrinsic was only codegen'd into the panic_unwind crate, but now it'll also be codegen'd when using panic=abort.

I don't think that this is a huge huge problem but I do think that we very much want to avoid any invoke instructions at all in panic=abort mode. Some targets in LLVM die really quickly on the invoke instruction I think. Could you double check the raw IR output in debug mode in panic=abort mode to make sure no invoke instruction exists?

This may require updating the codegen of the try intrinsic to just be call in panic=abort mode which I suspect would be a relatively simple change. It may also already be done, I'm not sure!

alexcrichton · 2020-01-06T18:45:50Z

src/libstd/panicking.rs


+// This must be kept in sync with the implementations in libpanic_unwind.
+//
+// This is *not* checked in anyway; the compiler does not allow us to use a


s/anyway/any way/

alexcrichton · 2020-01-06T18:46:10Z

src/libstd/panicking.rs

+// type/macro/anything from panic_unwind, since we're then linking in the
+// panic_unwind runtime even during -Cpanic=abort.
+//
+// Essentially this must be the type of `imp::Payload` in libpanic_unwind.


Would it be possible to extract the definition of Payload to a foo.rs file which we could include with #[path] here?

src/test/ui/no-landing-pads.rs

Amanieu · 2020-01-06T20:50:52Z

This may require updating the codegen of the try intrinsic to just be call in panic=abort mode which I suspect would be a relatively simple change. It may also already be done, I'm not sure!

This is already the case: try lowers to call with -Z no-landing-pads (which panic=abort enables).

alexcrichton · 2020-01-06T22:38:39Z

Great! That's perfect

rust-highfive · 2020-01-07T21:58:28Z

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2020-01-07T15:54:02.4589818Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-07T15:54:02.4675058Z ##[command]git config gc.auto 0
2020-01-07T15:54:02.4759986Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-07T15:54:02.4829368Z ##[command]git config --get-all http.proxy
2020-01-07T15:54:02.4970119Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67502/merge:refs/remotes/pull/67502/merge
---
2020-01-07T15:58:40.4496895Z     Checking alloc v0.0.0 (/checkout/src/liballoc)
2020-01-07T15:58:40.7700380Z     Checking cfg-if v0.1.8
2020-01-07T15:58:40.8156581Z     Checking rustc-demangle v0.1.16
2020-01-07T15:58:41.2595695Z     Checking panic_abort v0.0.0 (/checkout/src/libpanic_abort)
2020-01-07T15:58:41.2967617Z error: couldn't read src/libpanic_abort/../libpanic_unwind/payload.rs: No such file or directory (os error 2)
2020-01-07T15:58:41.2967972Z   --> src/libpanic_abort/lib.rs:24:1
2020-01-07T15:58:41.2968240Z    |
2020-01-07T15:58:41.2968501Z 24 | include!("../libpanic_unwind/payload.rs");
2020-01-07T15:58:41.2969002Z    | 
2020-01-07T15:58:41.2969233Z   ::: <::core::macros::builtin::include macros>:1:1
2020-01-07T15:58:41.2969418Z    |
2020-01-07T15:58:41.2969418Z    |
2020-01-07T15:58:41.2969716Z 1  | ($ file : expr) => { { } } ; ($ file : expr,) => { { } } ;
2020-01-07T15:58:41.2970035Z    | ---------------------------------------------------------- in this expansion of `include!`
2020-01-07T15:58:41.2990782Z error: aborting due to previous error
2020-01-07T15:58:41.2996142Z 
2020-01-07T15:58:41.3048740Z error: could not compile `panic_abort`.
2020-01-07T15:58:41.3049040Z warning: build failed, waiting for other jobs to finish...
---
2020-01-07T15:58:42.8482419Z   local time: Tue Jan  7 15:58:42 UTC 2020
2020-01-07T15:58:43.1308741Z   network time: Tue, 07 Jan 2020 15:58:43 GMT
2020-01-07T15:58:43.1314377Z == end clock drift check ==
2020-01-07T15:58:44.4231747Z 
2020-01-07T15:58:44.4340624Z ##[error]Bash exited with code '1'.
2020-01-07T15:58:44.4370787Z ##[section]Starting: Checkout
2020-01-07T15:58:44.4373469Z ==============================================================================
2020-01-07T15:58:44.4373544Z Task         : Get sources
2020-01-07T15:58:44.4373593Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

nagisa · 2020-03-07T14:09:32Z

src/libpanic_unwind/seh.rs

-            if (*e)[0] != 0 {
-                cleanup(*e);
+        unsafe extern $abi fn exception_cleanup(e: *mut Exception) {
+            if let Some(b) = e.read().data {


Nit: this is prone to diverging if anybody adds fields to Exception. Consider pattern matching the field out instead.

nagisa · 2020-03-07T14:15:51Z

src/librustc_codegen_llvm/intrinsic.rs

        //
        //   bx:
-        //      invoke %func(%args...) normal %normal unwind %catch
+        //      invoke %func(%data) normal %normal unwind %catch


nit: invoke %try_func.

Amanieu · 2020-03-12T17:35:49Z

Beta has been branched, so...

@bors r=Mark-Simulacrum

bors · 2020-03-12T17:35:53Z

📌 Commit 9f3679f has been approved by Mark-Simulacrum

Amanieu · 2020-03-12T17:36:06Z

@bors rollup=never

bors · 2020-03-12T19:37:04Z

⌛ Testing commit 9f3679f with merge 40f1118e156cefac66e96f41ac5be86adfb183ff...

bors · 2020-03-12T23:52:03Z

💥 Test timed out

Amanieu · 2020-03-13T17:21:58Z

@bors retry

Dylan-DPC-zz · 2020-03-13T18:07:44Z

@bors p=1

bors · 2020-03-13T22:43:12Z

⌛ Testing commit 9f3679f with merge be055d9...

bors · 2020-03-14T02:02:20Z

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing be055d9 to master...

RalfJung · 2020-03-14T09:36:40Z

src/libstd/panicking.rs

@@ -274,27 +269,59 @@ pub unsafe fn r#try<R, F: FnOnce() -> R>(f: F) -> Result<R, Box<dyn Any + Send>>
    //


Looks like the "2." comment above is no longer accurate? The code now does ManuallyDrop::into_inner(data.p) in the "panicked" case, which looks very symmetric to the "didn't panic case": in both cases we move "it out of data and return it", it's just a different thing we move out.

RalfJung · 2020-03-14T10:38:21Z

r=Mark-Simulacrum

Now Mark is both author and reviewer of this PR. ;)

EDIT: Ah, most commits are authored by @Amanieu, looks like Mark just submitted it. I see.

eggyal · 2021-04-07T15:45:29Z

src/libpanic_abort/lib.rs

+    #[rustc_std_internal_symbol]
+    #[cfg(all(bootstrap, target_os = "windows", target_env = "gnu"))]
    pub extern "C" fn rust_eh_unwind_resume() {}



This additional requirement on #cfg(bootstrap)] looks as though it may be responsible for #79609 ... not sure I understand why it was required, however ?

The compiler used to emit references to rust_eh_unwind_resume from codegen. But it doesn't do that any more and rust_eh_unwind_resume doesn't exist any more.

rust-highfive assigned shepmaster Dec 22, 2019

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

rust-highfive assigned alexcrichton and unassigned shepmaster Dec 22, 2019

Mark-Simulacrum mentioned this pull request Dec 22, 2019

Why is the code size of catch_unwind so large ? #64224

Closed

This comment has been minimized.

Sign in to view

Mark-Simulacrum force-pushed the opt-catch branch 3 times, most recently from afcbceb to 5de0856 Compare December 26, 2019 15:05

Mark-Simulacrum changed the title ~~Overlap caught panic with closure data~~ Optimize catch_unwind to match C++ try/catch Dec 26, 2019

tesuji reviewed Dec 26, 2019

View reviewed changes

src/libpanic_unwind/emcc.rs Outdated Show resolved Hide resolved

Amanieu reviewed Dec 26, 2019

View reviewed changes

Mark-Simulacrum force-pushed the opt-catch branch from 7da6a5a to 8ff0922 Compare December 26, 2019 16:15

This comment has been minimized.

Sign in to view

Mark-Simulacrum force-pushed the opt-catch branch from 8bb8bfe to ba58f64 Compare December 27, 2019 14:34

This comment has been minimized.

Sign in to view

Amanieu force-pushed the opt-catch branch from 6269792 to 5be658a Compare December 29, 2019 23:12

alexcrichton reviewed Jan 6, 2020

View reviewed changes

src/test/ui/no-landing-pads.rs Outdated Show resolved Hide resolved

nagisa reviewed Mar 7, 2020

View reviewed changes

Apply review feedback

9f3679f

RalfJung reviewed Mar 14, 2020

View reviewed changes

RalfJung mentioned this pull request Mar 14, 2020

adjust Miri to needs of changed unwinding strategy #69999

Merged

petrochenkov mentioned this pull request Mar 14, 2020

[x86_64-pc-windows-gnu] Nightly regression: rustc hangs on multiple tests from the UI test suite #70001

Closed

Amanieu mentioned this pull request Mar 16, 2020

Add a workaround for catch_unwind in stage1 mingw target #70010

Merged

Lesiuk mentioned this pull request Mar 19, 2020

LLVM generates incorrect code with -Zprofile #70148

Closed

Amanieu mentioned this pull request Mar 20, 2020

Remove -Z no-landing-pads flag #70175

Merged

elichai mentioned this pull request Mar 29, 2020

Fix broken benchmarks rust-bitcoin/rust-secp256k1#205

Merged

tmiasko mentioned this pull request Mar 31, 2020

i686-pc-windows-gnu fails to link on appveyor #70316

Closed

mati865 mentioned this pull request Apr 12, 2020

i686-pc-windows-gnu executables depend on additional DLL #71061

Closed

mati865 mentioned this pull request Sep 25, 2020

Late link args order #77208

Merged

glandium mentioned this pull request Oct 13, 2020

Rust sometimes emits calls to _Unwind_Resume on opt-level = 0 with panic = abort #53301

Closed

cynecx mentioned this pull request Oct 16, 2020

unwrap_panicked test fails (unwind support) cynecx/libfringe#1

Open

yutannihilation mentioned this pull request Jan 21, 2021

Duplicating libgcc.a as libgcc_eh.a for both mingw64/mingw32 to solve -lgcc_eh linking problem r-windows/rtools-packages#186

Merged

mati865 mentioned this pull request Apr 7, 2021

Passing -C panic=abort still attempts to link in libunwind when targeting i686-pc-windows-gnu on v1.44+ #79609

Open

eggyal reviewed Apr 7, 2021

View reviewed changes

Amanieu mentioned this pull request Jul 15, 2022

EXPERIMENT: support well-nested unwinds #99260

Closed

ayax79 mentioned this pull request Feb 2, 2024

Prevent Commands from killing nushell on panic nushell/nushell#11699

Closed

		@@ -274,27 +269,59 @@ pub unsafe fn r#try<R, F: FnOnce() -> R>(f: F) -> Result<R, Box<dyn Any + Send>>
		//

Uh oh!

Conversation

Mark-Simulacrum commented Dec 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Dec 22, 2019

Uh oh!

Mark-Simulacrum commented Dec 22, 2019

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

Mark-Simulacrum commented Dec 27, 2019

Uh oh!

Mark-Simulacrum commented Dec 28, 2019

Uh oh!

bors commented Dec 28, 2019

Uh oh!

bors commented Dec 28, 2019

Uh oh!

Amanieu commented Dec 29, 2019

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

alexcrichton Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!

alexcrichton Jan 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Amanieu commented Jan 6, 2020

Uh oh!

alexcrichton commented Jan 6, 2020

Uh oh!

rust-highfive commented Jan 7, 2020

Uh oh!

nagisa Mar 7, 2020 • 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!

nagisa Mar 7, 2020

Choose a reason for hiding this comment

Uh oh!

Amanieu commented Mar 12, 2020

Uh oh!

bors commented Mar 12, 2020

Uh oh!

Amanieu commented Mar 12, 2020

Uh oh!

bors commented Mar 12, 2020

Uh oh!

bors commented Mar 12, 2020

Uh oh!

Amanieu commented Mar 13, 2020

Uh oh!

Dylan-DPC-zz commented Mar 13, 2020

Uh oh!

bors commented Mar 13, 2020

Uh oh!

bors commented Mar 14, 2020

Uh oh!

RalfJung Mar 14, 2020

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eggyal Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

Amanieu Apr 7, 2021

Choose a reason for hiding this comment

Mark-Simulacrum commented Dec 22, 2019 •

edited

Loading

nagisa Mar 7, 2020 •

edited

Loading

RalfJung commented Mar 14, 2020 •

edited

Loading