From 11bb5c41eb98d8e7d4d75dfcf620f6f627523e77 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Wed, 26 May 2021 09:27:54 -0400 Subject: [PATCH] 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 --- gdb/ChangeLog | 5 +++++ gdb/regcache.c | 7 +++++-- 2 files changed, 10 insertions(+), 2 deletions(-) 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 + + * regcache.c (reg_buffer::reg_buffer): Default-initialize + m_registers array. + 2021-05-26 Tom Tromey * 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)] ()); } -- 2.30.2