Re: [PATCH 2/2] [gdb] Fix rethrow exception slicing in insert_bp_location

[Tom de Vries <tdevries@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2470 bytes --]

On 10/24/22 18:48, Simon Marchi wrote:
> On 10/24/22 12:43, Tom de Vries via Gdb-patches wrote:
>> On 10/24/22 18:36, Pedro Alves wrote:
>>> On 2022-10-24 9:49 a.m., Tom de Vries via Gdb-patches wrote:
>>>
>>>> +#define RETHROW_ON_TARGET_CLOSE_ERROR(E)                \
>>>> +  do                                    \
>>>> +    {                                    \
>>>> +      if ((E).reason != 0)                        \
>>>> +    {                                \
>>>> +      /* Can't set the breakpoint.  */                \
>>>> +                                    \
>>>> +      if ((E).error == TARGET_CLOSE_ERROR)                \
>>>> +        /* If the target has closed then it will have deleted any    \
>>>> +           breakpoints inserted within the target inferior, as a    \
>>>> +           result any further attempts to interact with the        \
>>>> +           breakpoint objects is not possible.  Just rethrow the    \
>>>> +           error.  Don't use E to rethrow, to prevent object    \
>>>> +           slicing of the exception.  */                \
>>>> +        throw;                            \
>>>> +    }                                \
>>>> +    } while (0)
>>>
>>> Is there a reason this is a macro instead of a function?
>>
>> yes, the throw without expression.
>>
>> Do you know of a way to do this using a function?
> 
> Maybe by passing the exception object as a parameter and using:
> 
> https://en.cppreference.com/w/cpp/error/rethrow_exception
> 
> ?

Thanks for pointing out this possibility, I've tried this out, as attached.

I like the fact that we no longer throw something implicitly defined by 
a catch outside the function, which looks a bit awkward to me.

But sofar, I didn't manage to understand from reading the specifications 
of std::current_exception and std::rethrow_exception whether there will 
be copying of the exception or not, and AFAICT only the throw without 
expression guarantees to reuse the existing exception object, so I'm 
going with that instead (which I'll post in reply to Pedro).

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-Rewrite-RETHROW_ON_TARGET_CLOSE_ERROR-into-funct.patch --]
[-- Type: text/x-patch, Size: 3579 bytes --]

From 48ae602c38c8b879baa76a7378176bc9b80f8662 Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Tue, 25 Oct 2022 08:17:46 +0200
Subject: [PATCH] [gdb] Rewrite RETHROW_ON_TARGET_CLOSE_ERROR into function

Recent commit b2829fcf9b5 ("[gdb] Fix rethrow exception slicing in
insert_bp_location") introduced macro RETHROW_ON_TARGET_CLOSE_ERROR.

I wrote this as a macro in order to have the rethrowing throw be part of the
same function as the catch, but as it turns out that's not necessary.

Rewrite into a function.

Rebuild on x86_64-linux and tested gdb.server/server-kill.exp, which
triggers the rethrow.
---
 gdb/breakpoint.c | 45 +++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index a001e78cfb4..ca055aa5398 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2647,23 +2647,28 @@ breakpoint_kind (const struct bp_location *bl, CORE_ADDR *addr)
     return gdbarch_breakpoint_kind_from_pc (bl->gdbarch, addr);
 }
 
-#define RETHROW_ON_TARGET_CLOSE_ERROR(E)				\
-  do									\
-    {									\
-      if ((E).reason != 0)						\
-	{								\
-	  /* Can't set the breakpoint.  */				\
-									\
-	  if ((E).error == TARGET_CLOSE_ERROR)				\
-	    /* If the target has closed then it will have deleted any	\
-	       breakpoints inserted within the target inferior, as a	\
-	       result any further attempts to interact with the		\
-	       breakpoint objects is not possible.  Just rethrow the	\
-	       error.  Don't use E to rethrow, to prevent object	\
-	       slicing of the exception.  */				\
-	    throw;							\
-	}								\
-    } while (0)
+/* Rethrow the currently handled exception, if it's a TARGET_CLOSE_ERROR.
+   E is either the currently handled exception, or a copy, or a sliced copy,
+   so we can't rethrow that one, but we can use it to inspect the properties
+   of the currently handled exception.  Instead, rethrow EPTR.  */
+
+static void
+rethrow_on_target_close_error (const gdb_exception &e, std::exception_ptr eptr)
+{
+  if (e.reason == 0)
+    return;
+  /* Can't set the breakpoint.  */
+
+  if (e.error != TARGET_CLOSE_ERROR)
+    return;
+
+  /* If the target has closed then it will have deleted any breakpoints
+     inserted within the target inferior, as a result any further attempts
+     to interact with the breakpoint objects is not possible.  Just rethrow
+     the error.  Don't use e to rethrow, to prevent object slicing of the
+     exception.  */
+  std::rethrow_exception (eptr);
+}
 
 /* Insert a low-level "breakpoint" of some type.  BL is the breakpoint
    location.  Any error messages are printed to TMP_ERROR_STREAM; and
@@ -2752,7 +2757,7 @@ insert_bp_location (struct bp_location *bl,
 	    }
 	  catch (gdb_exception &e)
 	    {
-	      RETHROW_ON_TARGET_CLOSE_ERROR (e);
+	      rethrow_on_target_close_error (e, std::current_exception());
 	      bp_excpt = std::move (e);
 	    }
 	}
@@ -2792,7 +2797,7 @@ insert_bp_location (struct bp_location *bl,
 		    }
 		  catch (gdb_exception &e)
 		    {
-		      RETHROW_ON_TARGET_CLOSE_ERROR (e);
+		      rethrow_on_target_close_error (e, std::current_exception());
 		      bp_excpt = std::move (e);
 		    }
 
@@ -2817,7 +2822,7 @@ insert_bp_location (struct bp_location *bl,
 		}
 	      catch (gdb_exception &e)
 		{
-		  RETHROW_ON_TARGET_CLOSE_ERROR (e);
+		  rethrow_on_target_close_error (e, std::current_exception());
 		  bp_excpt = std::move (e);
 		}
 	    }

base-commit: c6d20401a20148b032cb7fb9dba079466a9383cc
-- 
2.35.3