* [PATCH] gdbsupport: fix scoped_debug_start_end's move constructor
@ 2023-01-04 21:22 Simon Marchi
2023-01-04 22:01 ` Simon Marchi
0 siblings, 1 reply; 4+ messages in thread
From: Simon Marchi @ 2023-01-04 21:22 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess, Simon Marchi
I spotted a problem with scoped_debug_start_end's move constructor.
When constructing a scoped_debug_start_end through it, it doesn't
disable the moved-from object, meaning there are now two objects that
will do the side-effects of decrementing the debug_print_depth global
and printing the "end" message. Decrementing the debug_print_depth
global twice is actually problematic, because the increments and
decrements get out of sync, meaning we should hit this assertion, in
theory:
gdb_assert (debug_print_depth > 0);
However, in practice, we don't see that. This is because despite the
move constructor being required for this to compile:
template<typename PT>
static inline scoped_debug_start_end<PT &> ATTRIBUTE_NULL_PRINTF (6, 7)
make_scoped_debug_start_end (PT &&pred, const char *module, const char *func,
const char *start_prefix,
const char *end_prefix, const char *fmt, ...)
{
va_list args;
va_start (args, fmt);
auto res = scoped_debug_start_end<PT &> (pred, module, func, start_prefix,
end_prefix, fmt, args);
va_end (args);
return res;
}
... it is never actually called, because compilers elide the move
constructors all the way (the scoped_debug_start_end gets constructed
directly in the instance of the top-level caller). To confirm this, I
built GDB with -fno-elide-constructors, and now I see it:
/home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h:147: internal-error: ~scoped_debug_start_end: Assertion `debug_print_depth > 0' failed.
#9 0x00005614ba5f17c3 in internal_error_loc (file=0x5614b8749960 "/home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h", line=147, fmt=0x5614b8733fa0 "%s: Assertion `%s' failed.") at /home/simark/src/binutils-gdb/gdbsupport/errors.cc:58
#10 0x00005614b8e1b2e5 in scoped_debug_start_end<bool&>::~scoped_debug_start_end (this=0x7ffc6c5e7b40, __in_chrg=<optimized out>) at /home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h:147
#11 0x00005614b96dbe34 in make_scoped_debug_start_end<bool&> (pred=@0x5614baad7200: true, module=0x5614b891d840 "infrun", func=0x5614b891d800 "infrun_debug_show_threads", start_prefix=0x5614b891d7c0 "enter", end_prefix=0x5614b891d780 "exit", fmt=0x0) at /home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h:235
Fix this by adding an m_disabled field to scoped_debug_start_end, and
setting it in the move constructor.
Change-Id: Ie5213269c584837f751d2d11de831f45ae4a899f
---
gdbsupport/common-debug.h | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/gdbsupport/common-debug.h b/gdbsupport/common-debug.h
index ec36d88fdea2..33b15a005f11 100644
--- a/gdbsupport/common-debug.h
+++ b/gdbsupport/common-debug.h
@@ -138,10 +138,25 @@ struct scoped_debug_start_end
DISABLE_COPY_AND_ASSIGN (scoped_debug_start_end);
- scoped_debug_start_end (scoped_debug_start_end &&other) = default;
+ scoped_debug_start_end (scoped_debug_start_end &&other)
+ : m_debug_enabled (other.m_debug_enabled),
+ m_module (other.m_module),
+ m_func (other.m_func),
+ m_end_prefix (other.m_end_prefix),
+ m_msg (other.m_msg),
+ m_with_format (other.m_with_format),
+ m_must_decrement_print_depth (other.m_must_decrement_print_depth),
+ m_disabled (other.m_disabled)
+ {
+ /* Avoid the moved-from object doing the side-effects in its destructor. */
+ other.m_disabled = true;
+ }
~scoped_debug_start_end ()
{
+ if (m_disabled)
+ return;
+
if (m_must_decrement_print_depth)
{
gdb_assert (debug_print_depth > 0);
@@ -194,6 +209,10 @@ struct scoped_debug_start_end
construction but not during destruction, or vice-versa. We want to make
sure there are as many increments are there are decrements. */
bool m_must_decrement_print_depth = false;
+
+ /* True if this object was moved from, and the destructor behavior must be
+ inhibited. */
+ bool m_disabled = false;
};
/* Implementation of is_debug_enabled when PT is an invokable type. */
base-commit: aa036eccf094c4d85b953cf1cc2892f0ff746fd9
--
2.39.0
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gdbsupport: fix scoped_debug_start_end's move constructor
2023-01-04 21:22 [PATCH] gdbsupport: fix scoped_debug_start_end's move constructor Simon Marchi
@ 2023-01-04 22:01 ` Simon Marchi
2023-01-05 20:05 ` Tom Tromey
0 siblings, 1 reply; 4+ messages in thread
From: Simon Marchi @ 2023-01-04 22:01 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
On 1/4/23 16:22, Simon Marchi wrote:
> I spotted a problem with scoped_debug_start_end's move constructor.
> When constructing a scoped_debug_start_end through it, it doesn't
> disable the moved-from object, meaning there are now two objects that
> will do the side-effects of decrementing the debug_print_depth global
> and printing the "end" message. Decrementing the debug_print_depth
> global twice is actually problematic, because the increments and
> decrements get out of sync, meaning we should hit this assertion, in
> theory:
>
> gdb_assert (debug_print_depth > 0);
>
> However, in practice, we don't see that. This is because despite the
> move constructor being required for this to compile:
>
> template<typename PT>
> static inline scoped_debug_start_end<PT &> ATTRIBUTE_NULL_PRINTF (6, 7)
> make_scoped_debug_start_end (PT &&pred, const char *module, const char *func,
> const char *start_prefix,
> const char *end_prefix, const char *fmt, ...)
> {
> va_list args;
> va_start (args, fmt);
> auto res = scoped_debug_start_end<PT &> (pred, module, func, start_prefix,
> end_prefix, fmt, args);
> va_end (args);
>
> return res;
> }
>
> ... it is never actually called, because compilers elide the move
> constructors all the way (the scoped_debug_start_end gets constructed
> directly in the instance of the top-level caller). To confirm this, I
> built GDB with -fno-elide-constructors, and now I see it:
>
> /home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h:147: internal-error: ~scoped_debug_start_end: Assertion `debug_print_depth > 0' failed.
>
> #9 0x00005614ba5f17c3 in internal_error_loc (file=0x5614b8749960 "/home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h", line=147, fmt=0x5614b8733fa0 "%s: Assertion `%s' failed.") at /home/simark/src/binutils-gdb/gdbsupport/errors.cc:58
> #10 0x00005614b8e1b2e5 in scoped_debug_start_end<bool&>::~scoped_debug_start_end (this=0x7ffc6c5e7b40, __in_chrg=<optimized out>) at /home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h:147
> #11 0x00005614b96dbe34 in make_scoped_debug_start_end<bool&> (pred=@0x5614baad7200: true, module=0x5614b891d840 "infrun", func=0x5614b891d800 "infrun_debug_show_threads", start_prefix=0x5614b891d7c0 "enter", end_prefix=0x5614b891d780 "exit", fmt=0x0) at /home/simark/src/binutils-gdb/gdb/../gdbsupport/common-debug.h:235
>
> Fix this by adding an m_disabled field to scoped_debug_start_end, and
> setting it in the move constructor.
>
> Change-Id: Ie5213269c584837f751d2d11de831f45ae4a899f
> ---
> gdbsupport/common-debug.h | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/gdbsupport/common-debug.h b/gdbsupport/common-debug.h
> index ec36d88fdea2..33b15a005f11 100644
> --- a/gdbsupport/common-debug.h
> +++ b/gdbsupport/common-debug.h
> @@ -138,10 +138,25 @@ struct scoped_debug_start_end
>
> DISABLE_COPY_AND_ASSIGN (scoped_debug_start_end);
>
> - scoped_debug_start_end (scoped_debug_start_end &&other) = default;
> + scoped_debug_start_end (scoped_debug_start_end &&other)
> + : m_debug_enabled (other.m_debug_enabled),
> + m_module (other.m_module),
> + m_func (other.m_func),
> + m_end_prefix (other.m_end_prefix),
> + m_msg (other.m_msg),
Just found this nit... not that it changes anything (because this ctor
isn't called in practice), but we should std::move m_msg. I'll change
it locally.
Well, we could std::move all fields, but it would be a bit verbose.
Simon
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gdbsupport: fix scoped_debug_start_end's move constructor
2023-01-04 22:01 ` Simon Marchi
@ 2023-01-05 20:05 ` Tom Tromey
2023-01-05 20:23 ` Simon Marchi
0 siblings, 1 reply; 4+ messages in thread
From: Tom Tromey @ 2023-01-05 20:05 UTC (permalink / raw)
To: Simon Marchi via Gdb-patches; +Cc: Simon Marchi, Andrew Burgess
>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>> + scoped_debug_start_end (scoped_debug_start_end &&other)
>> + : m_debug_enabled (other.m_debug_enabled),
>> + m_module (other.m_module),
>> + m_func (other.m_func),
>> + m_end_prefix (other.m_end_prefix),
>> + m_msg (other.m_msg),
Simon> Just found this nit... not that it changes anything (because this ctor
Simon> isn't called in practice), but we should std::move m_msg. I'll change
Simon> it locally.
I think it's also fine to just leave it as-is.
Simon> Well, we could std::move all fields, but it would be a bit verbose.
If we think we may need this kind of behavior again, one way would be a
sort of "move token" object that wraps a bool, and that sets the bool on
construction and clears it on move. Then scoped_debug_start_end could
just use the default move constructor again, and check the token's value
in its destructor.
Probably overkill for just the one case. I think your patch is ok.
Tom
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gdbsupport: fix scoped_debug_start_end's move constructor
2023-01-05 20:05 ` Tom Tromey
@ 2023-01-05 20:23 ` Simon Marchi
0 siblings, 0 replies; 4+ messages in thread
From: Simon Marchi @ 2023-01-05 20:23 UTC (permalink / raw)
To: Tom Tromey, Simon Marchi via Gdb-patches; +Cc: Andrew Burgess
On 1/5/23 15:05, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>>> + scoped_debug_start_end (scoped_debug_start_end &&other)
>>> + : m_debug_enabled (other.m_debug_enabled),
>>> + m_module (other.m_module),
>>> + m_func (other.m_func),
>>> + m_end_prefix (other.m_end_prefix),
>>> + m_msg (other.m_msg),
>
> Simon> Just found this nit... not that it changes anything (because this ctor
> Simon> isn't called in practice), but we should std::move m_msg. I'll change
> Simon> it locally.
>
> I think it's also fine to just leave it as-is.
>
> Simon> Well, we could std::move all fields, but it would be a bit verbose.
>
> If we think we may need this kind of behavior again, one way would be a
> sort of "move token" object that wraps a bool, and that sets the bool on
> construction and clears it on move. Then scoped_debug_start_end could
> just use the default move constructor again, and check the token's value
> in its destructor.
Interesting, I'll keep this in mind. I think it could apply to some
scoped_restore_* objects that have global side effects, that we want to
be movable, because we want to be able to have functions that return
them.
> Probably overkill for just the one case. I think your patch is ok.
Ok, thanks, will push.
Simon
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-05 20:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-04 21:22 [PATCH] gdbsupport: fix scoped_debug_start_end's move constructor Simon Marchi
2023-01-04 22:01 ` Simon Marchi
2023-01-05 20:05 ` Tom Tromey
2023-01-05 20:23 ` Simon Marchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).