From: Simon Marchi <simon.marchi@efficios.com>
Date: Wed, 26 May 2021 13:27:54 +0000 (-0400)
Subject: gdb: don't zero-initialize reg_buffer contents
X-Git-Tag: binutils-2_37~525
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=11bb5c41eb98d8e7d4d75dfcf620f6f627523e77;p=thirdparty%2Fbinutils-gdb.git

gdb: don't zero-initialize reg_buffer contents

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
---

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3efdb5d05e8..39f444e1f78 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2021-05-26  Simon Marchi  <simon.marchi@efficios.com>
+
+	* regcache.c (reg_buffer::reg_buffer): Default-initialize
+	m_registers array.
+
 2021-05-26  Tom Tromey  <tom@tromey.com>
 
 	* dwarf2/read.c (allocate_type_unit_groups_table)
diff --git a/gdb/regcache.c b/gdb/regcache.c
index d2386308b82..51e9effe642 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -184,15 +184,18 @@ reg_buffer::reg_buffer (gdbarch *gdbarch, bool has_pseudo)
   gdb_assert (gdbarch != NULL);
   m_descr = regcache_descr (gdbarch);
 
+  /* We don't zero-initialize the M_REGISTERS array, as the bytes it contains
+     aren't meaningful as long as the corresponding register status is not
+     REG_VALID.  */
   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)] ());
     }