diff options
author | Simon Marchi <simon.marchi@efficios.com> | 2021-05-26 09:27:54 -0400 |
---|---|---|
committer | Simon Marchi <simon.marchi@polymtl.ca> | 2021-05-26 15:03:00 -0400 |
commit | 11bb5c41eb98d8e7d4d75dfcf620f6f627523e77 (patch) | |
tree | 67c712afe801e1e8bc19e9cafa00224e16bf89f1 | |
parent | 983d5689cc07466ddc6664e10aebbb2b2fd49515 (diff) | |
download | gdb-11bb5c41eb98d8e7d4d75dfcf620f6f627523e77.zip gdb-11bb5c41eb98d8e7d4d75dfcf620f6f627523e77.tar.gz gdb-11bb5c41eb98d8e7d4d75dfcf620f6f627523e77.tar.bz2 |
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
-rw-r--r-- | gdb/ChangeLog | 5 | ||||
-rw-r--r-- | gdb/regcache.c | 7 |
2 files changed, 10 insertions, 2 deletions
diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 3efdb5d..39f444e 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 d238630..51e9eff 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)] ()); } |