[2/2] Fix rethrow exception slicing in insert_bp_location
Commit Message
The preferred way of rethrowing an exception is by using throw without
expression, because it avoids object slicing of the exception [1].
Fix this in insert_bp_location.
[1] https://en.cppreference.com/w/cpp/language/throw
---
gdb/breakpoint.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
Comments
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?
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?
Thanks,
- Tom
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
?
Simon
On 2022-10-24 5:43 p.m., Tom de Vries 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?
WDYM? I believe it should Just Work. E.g.:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#include <stdio.h>
struct excpt
{
};
struct excpt2 : excpt
{
};
void
rethrow ()
{
throw;
}
int main ()
{
try
{
try
{
throw excpt2{};
}
catch (const excpt &)
{
rethrow ();
}
}
catch (const excpt2 &)
{
printf ("caught excpt2\n");
}
return 0;
}
$ g++ test_rethrow.cc -o test_rethrow
$ ./test_rethrow
caught excpt2
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
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
On 10/24/22 18:51, Pedro Alves wrote:
>
>
> On 2022-10-24 5:43 p.m., Tom de Vries 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?
>
> WDYM? I believe it should Just Work. E.g.:
>
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> #include <stdio.h>
>
> struct excpt
> {
> };
>
> struct excpt2 : excpt
> {
> };
>
> void
> rethrow ()
> {
> throw;
> }
>
> int main ()
> {
> try
> {
> try
> {
> throw excpt2{};
> }
> catch (const excpt &)
> {
> rethrow ();
> }
> }
> catch (const excpt2 &)
> {
> printf ("caught excpt2\n");
> }
> return 0;
> }
>
> $ g++ test_rethrow.cc -o test_rethrow
> $ ./test_rethrow
> caught excpt2
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Hmm, it does indeed, I didn't realize this was possible.
Fixed in attached patch.
Any further comments?
Thanks,
- Tom
On 2022-10-25 8:14 a.m., Tom de Vries wrote:
> On 10/24/22 18:51, Pedro Alves wrote:
>> On 2022-10-24 5:43 p.m., Tom de Vries 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:
>>>> 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?
>>
>> WDYM? I believe it should Just Work. E.g.:
...
> Hmm, it does indeed, I didn't realize this was possible.
>
> Fixed in attached patch.
>
> Any further comments?
Nope, looks fine.
Thanks,
Pedro Alves
@@ -2647,6 +2647,24 @@ 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)
+
/* Insert a low-level "breakpoint" of some type. BL is the breakpoint
location. Any error messages are printed to TMP_ERROR_STREAM; and
DISABLED_BREAKS, and HW_BREAKPOINT_ERROR are used to report problems.
@@ -2734,6 +2752,7 @@ insert_bp_location (struct bp_location *bl,
}
catch (gdb_exception &e)
{
+ RETHROW_ON_TARGET_CLOSE_ERROR (e);
bp_excpt = std::move (e);
}
}
@@ -2773,6 +2792,7 @@ insert_bp_location (struct bp_location *bl,
}
catch (gdb_exception &e)
{
+ RETHROW_ON_TARGET_CLOSE_ERROR (e);
bp_excpt = std::move (e);
}
@@ -2797,6 +2817,7 @@ insert_bp_location (struct bp_location *bl,
}
catch (gdb_exception &e)
{
+ RETHROW_ON_TARGET_CLOSE_ERROR (e);
bp_excpt = std::move (e);
}
}
@@ -2811,13 +2832,6 @@ insert_bp_location (struct bp_location *bl,
if (bp_excpt.reason != 0)
{
/* Can't set the breakpoint. */
-
- /* 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. */
- if (bp_excpt.error == TARGET_CLOSE_ERROR)
- throw bp_excpt;
gdb_assert (bl->owner != nullptr);
/* In some cases, we might not be able to insert a