FreeChainXenon/binutils-gdb
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

gdb: don't zero-initialize reg_buffer contents

Browse source 
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 835dcf9261 ("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
This commit is contained in:
Simon Marchi 2021-05-26 09:27:54 -04:00 • committed by Simon Marchi
parent 983d5689cc
commit 11bb5c41eb
2 changed files with 10 additions and 2 deletions
Download patch file Download diff file Expand all files Collapse all files

7
gdb/regcache.c
View file

@ -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)] ());
}