[PATCH] gdb: don't zero-initialize reg_buffer contents

[PATCH] gdb: don't zero-initialize reg_buffer contents

[Simon Marchi]

From: Simon Marchi <simon.marchi@efficios.com>

The reg_buffer constructor zero-initializes (value-initializes, in C++
speak) the gdb_bytes of the m_registers array.  This is not necessary,
as these bytes are only meaningful if the corresponding register_status
is REG_VALID.  If the corresponding register_status is REG_VALID, then
they will have been overwritten with the actual register data when
reading the registers from the system into the reg_buffer.

Fix that by removing the empty parenthesis following the new expression,
meaning that the bytes will now be default-initialized, meaning they'll
be left uninitialized.  For reference, this is explained here:

  https://en.cppreference.com/w/cpp/language/new#Construction

These new expressions were added in 835dcf92618e ("Use std::unique_ptr
in reg_buffer").  As mentioned in that commit message, the use of
value-initialisation was done on purpose to keep existing behavior, but
now there is some data that suggest it would be beneficial not to do it,
which is why I suggest changing it.

This doesn't make a big difference on typical architectures where the
register buffer is not that big.  However, on ROCm (AMD GPU), the
register buffer is about 65000 bytes big, so the reg_buffer constructor
shows up in profiling.  If you want to make some tests and profile it on
a standard system, it's always possible to change:

  - m_registers.reset (new gdb_byte[m_descr->sizeof_raw_registers] ());
  + m_registers.reset (new gdb_byte[65000] ());

and run a program that constantly hits a breakpoint with a false
condition.  For example, by doing this change and running the following
program:

    static void break_here () {}

    int main ()
    {
      for (int i = 0; i < 100000; i++)
        break_here ();
    }

with the following GDB incantation:

   /usr/bin/time  ./gdb -nx --data-directory=data-directory  -q test -ex "b break_here if 0" -ex r -batch

I get, for value-intializing:

    11.75user 7.68system 0:18.54elapsed 104%CPU (0avgtext+0avgdata 56644maxresident)k

And for default-initializing:

    6.83user 8.42system 0:14.12elapsed 108%CPU (0avgtext+0avgdata 56512maxresident)k

gdb/ChangeLog:

	* regcache.c (reg_buffer::reg_buffer): Default-initialize
	m_registers array.

Change-Id: I5071a4444dee0530ce1bc58ebe712024ddd2b158
---
 gdb/regcache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/regcache.c b/gdb/regcache.c
index d2386308b822..3d804266e565 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -186,13 +186,13 @@ reg_buffer::reg_buffer (gdbarch *gdbarch, bool has_pseudo)
 
   if (has_pseudo)
     {
-      m_registers.reset (new gdb_byte[m_descr->sizeof_cooked_registers] ());
+      m_registers.reset (new gdb_byte[m_descr->sizeof_cooked_registers]);
       m_register_status.reset
 	(new register_status[m_descr->nr_cooked_registers] ());
     }
   else
     {
-      m_registers.reset (new gdb_byte[m_descr->sizeof_raw_registers] ());
+      m_registers.reset (new gdb_byte[m_descr->sizeof_raw_registers]);
       m_register_status.reset
 	(new register_status[gdbarch_num_regs (gdbarch)] ());
     }
-- 
2.31.1

Re: [PATCH] gdb: don't zero-initialize reg_buffer contents

[Andrew Burgess]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-05-25 18:43:49 -0400]:

> From: Simon Marchi <simon.marchi@efficios.com>
> 
> The reg_buffer constructor zero-initializes (value-initializes, in C++
> speak) the gdb_bytes of the m_registers array.  This is not necessary,
> as these bytes are only meaningful if the corresponding register_status
> is REG_VALID.  If the corresponding register_status is REG_VALID, then
> they will have been overwritten with the actual register data when
> reading the registers from the system into the reg_buffer.
> 
> Fix that by removing the empty parenthesis following the new expression,
> meaning that the bytes will now be default-initialized, meaning they'll
> be left uninitialized.  For reference, this is explained here:
> 
>   https://en.cppreference.com/w/cpp/language/new#Construction
> 
> These new expressions were added in 835dcf92618e ("Use std::unique_ptr
> in reg_buffer").  As mentioned in that commit message, the use of
> value-initialisation was done on purpose to keep existing behavior, but
> now there is some data that suggest it would be beneficial not to do it,
> which is why I suggest changing it.
> 
> This doesn't make a big difference on typical architectures where the
> register buffer is not that big.  However, on ROCm (AMD GPU), the
> register buffer is about 65000 bytes big, so the reg_buffer constructor
> shows up in profiling.  If you want to make some tests and profile it on
> a standard system, it's always possible to change:
> 
>   - m_registers.reset (new gdb_byte[m_descr->sizeof_raw_registers] ());
>   + m_registers.reset (new gdb_byte[65000] ());
> 
> and run a program that constantly hits a breakpoint with a false
> condition.  For example, by doing this change and running the following
> program:
> 
>     static void break_here () {}
> 
>     int main ()
>     {
>       for (int i = 0; i < 100000; i++)
>         break_here ();
>     }
> 
> with the following GDB incantation:
> 
>    /usr/bin/time  ./gdb -nx --data-directory=data-directory  -q test -ex "b break_here if 0" -ex r -batch
> 
> I get, for value-intializing:
> 
>     11.75user 7.68system 0:18.54elapsed 104%CPU (0avgtext+0avgdata 56644maxresident)k
> 
> And for default-initializing:
> 
>     6.83user 8.42system 0:14.12elapsed 108%CPU (0avgtext+0avgdata 56512maxresident)k
> 
> gdb/ChangeLog:
> 
> 	* regcache.c (reg_buffer::reg_buffer): Default-initialize
> 	m_registers array.
> 
> Change-Id: I5071a4444dee0530ce1bc58ebe712024ddd2b158
> ---
>  gdb/regcache.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index d2386308b822..3d804266e565 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -186,13 +186,13 @@ reg_buffer::reg_buffer (gdbarch *gdbarch, bool has_pseudo)
>  
>    if (has_pseudo)
>      {
> -      m_registers.reset (new gdb_byte[m_descr->sizeof_cooked_registers] ());
> +      m_registers.reset (new gdb_byte[m_descr->sizeof_cooked_registers]);
>        m_register_status.reset
>  	(new register_status[m_descr->nr_cooked_registers] ());
>      }
>    else
>      {
> -      m_registers.reset (new gdb_byte[m_descr->sizeof_raw_registers] ());
> +      m_registers.reset (new gdb_byte[m_descr->sizeof_raw_registers]);
>        m_register_status.reset
>  	(new register_status[gdbarch_num_regs (gdbarch)] ());
>      }

Makes sense.  I wonder if adding a comment that the choice to leave
this data uninitialised is intentional would be useful?  There's no
test that will catch this if someone changes this back thinking, what
harm can it do, registers aren't _that_ big...

Up to you, but LGTM.

Thanks,
Andrew

Re: [PATCH] gdb: don't zero-initialize reg_buffer contents

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Makes sense.  I wonder if adding a comment that the choice to leave
Andrew> this data uninitialised is intentional would be useful?  There's no
Andrew> test that will catch this if someone changes this back thinking, what
Andrew> harm can it do, registers aren't _that_ big...

This sounds sensible to me as well.

Tom

Re: [PATCH] gdb: don't zero-initialize reg_buffer contents

[Simon Marchi]

On 2021-05-26 8:51 a.m., Tom Tromey wrote:
>>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> Makes sense.  I wonder if adding a comment that the choice to leave
> Andrew> this data uninitialised is intentional would be useful?  There's no
> Andrew> test that will catch this if someone changes this back thinking, what
> Andrew> harm can it do, registers aren't _that_ big...
> 
> This sounds sensible to me as well.

I would be surprised if this happened, but a comment is cheap, so in
doubt I'll add it (and push the patch).  Thanks!

Simon