From 757ed07f374edfe93be5c9084ac5c44ba738e1b2 Mon Sep 17 00:00:00 2001
From: Amanieu d'Antras <amanieu@gmail.com>
Date: Tue, 7 Jan 2020 11:36:57 +0100
Subject: [PATCH] Apply review feedback

---
 src/libpanic_unwind/emcc.rs            |  4 ++++
 src/libpanic_unwind/seh.rs             | 31 +++++++++++---------------
 src/librustc_codegen_llvm/intrinsic.rs |  6 ++++-
 3 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/src/libpanic_unwind/emcc.rs b/src/libpanic_unwind/emcc.rs
index 61f33fd4d5d..fbadf4ac6a0 100644
--- a/src/libpanic_unwind/emcc.rs
+++ b/src/libpanic_unwind/emcc.rs
@@ -53,6 +53,10 @@ pub fn payload() -> *mut u8 {
 }
 
 struct Exception {
+    // This needs to be an Option because the object's lifetime follows C++
+    // semantics: when catch_unwind moves the Box out of the exception it must
+    // still leave the exception object in a valid state because its destructor
+    // is still going to be called by __cxa_end_catch..
     data: Option<Box<dyn Any + Send>>,
 }
 
diff --git a/src/libpanic_unwind/seh.rs b/src/libpanic_unwind/seh.rs
index ef27e38c2d1..f1d0080472a 100644
--- a/src/libpanic_unwind/seh.rs
+++ b/src/libpanic_unwind/seh.rs
@@ -224,33 +224,28 @@ pub struct _TypeDescriptor {
 // used as the result of the exception copy. This is used by the C++ runtime to
 // support capturing exceptions with std::exception_ptr, which we can't support
 // because Box<dyn Any> isn't clonable.
-cfg_if::cfg_if! {
-    if #[cfg(target_arch = "x86")] {
-        unsafe extern "thiscall" fn exception_cleanup(e: *mut [u64; 2]) {
-            if (*e)[0] != 0 {
-                cleanup(*e);
-            }
-        }
-        #[unwind(allowed)]
-        unsafe extern "thiscall" fn exception_copy(_dest: *mut [u64; 2],
-                                                   _src: *mut [u64; 2])
-                                                   -> *mut [u64; 2] {
-            panic!("Rust panics cannot be copied");
-        }
-    } else {
-        unsafe extern "C" fn exception_cleanup(e: *mut [u64; 2]) {
+macro_rules! define_cleanup {
+    ($abi:tt) => {
+        unsafe extern $abi fn exception_cleanup(e: *mut [u64; 2]) {
             if (*e)[0] != 0 {
                 cleanup(*e);
             }
         }
         #[unwind(allowed)]
-        unsafe extern "C" fn exception_copy(_dest: *mut [u64; 2],
-                                            _src: *mut [u64; 2])
-                                            -> *mut [u64; 2] {
+        unsafe extern $abi fn exception_copy(_dest: *mut [u64; 2],
+                                             _src: *mut [u64; 2])
+                                             -> *mut [u64; 2] {
             panic!("Rust panics cannot be copied");
         }
     }
 }
+cfg_if::cfg_if! {
+   if #[cfg(target_arch = "x86")] {
+       define_cleanup!("thiscall");
+   } else {
+       define_cleanup!("C");
+   }
+}
 
 pub unsafe fn panic(data: Box<dyn Any + Send>) -> u32 {
     use core::intrinsics::atomic_store;
diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs
index 27308cabd45..031837c1efb 100644
--- a/src/librustc_codegen_llvm/intrinsic.rs
+++ b/src/librustc_codegen_llvm/intrinsic.rs
@@ -953,7 +953,9 @@ fn codegen_msvc_try(
         catchswitch.add_handler(cs, catchpad.llbb());
 
         // The flag value of 8 indicates that we are catching the exception by
-        // reference instead of by value.
+        // reference instead of by value. We can't use catch by value because
+        // that requires copying the exception object, which we don't support
+        // since our exception object effectively contains a Box.
         //
         // Source: MicrosoftCXXABI::getAddrOfCXXCatchHandlerType in clang
         let flags = bx.const_i32(8);
@@ -970,6 +972,8 @@ fn codegen_msvc_try(
         catchpad.store(payload, local_ptr, i64_align);
 
         // Clear the first word of the exception so avoid double-dropping it.
+        // This will be read by the destructor which is implicitly called at the
+        // end of the catch block by the runtime.
         let payload_0_ptr = catchpad.inbounds_gep(payload_ptr, &[bx.const_i32(0), bx.const_i32(0)]);
         catchpad.store(bx.const_u64(0), payload_0_ptr, i64_align);
 
-- 
2.44.0