Optimize catch_unwind to match C++ try/catch

[Mark-Simulacrum]

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]

r? @shepmaster

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

[Mark-Simulacrum]

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).

[Mark-Simulacrum]

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]

@bors try for easier testing

[bors]

⌛ Trying commit ba58f6421d4c5598f16479b0b2e5a1357917bac0 with merge d19982ed64d76225c52b773a53d593e6e96282e8...

[bors]

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

[Amanieu]

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]

s/anyway/any way/

[alexcrichton]

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

[Amanieu]

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]

Great! That's perfect

[rust-highfive]

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)

[bors]

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

[RalfJung]

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]

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]

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

[Amanieu]

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.