RE: [PATCHv6 04/10] gdb: build dprintf commands just once in code_breakpoint constructor

Aktemur, Tankut Baris
2023-12-05T08:18:08Z

On Saturday, December 2, 2023 11:42 AM, Andrew Burgess wrote:
> I noticed in code_breakpoint::code_breakpoint that we are calling
> update_dprintf_command_list once for each breakpoint location, when we
> really only need to call this once per breakpoint -- the data updated
> by this function, the breakpoint command list -- is per breakpoint,
> not per breakpoint location. Calling update_dprintf_command_list
> multiple times is just wasted effort, there's no per location error
> checking, we don't even pass the current location to the function.
>
> This commit moves the update_dprintf_command_list call outside of the
> per-location loop. I have also changes the 'if' that handles the case
> where the extra_string (which holds the format/args) is empty. I
> don't believe that this situation can ever arise -- and if it does we
> should be catching it earlier and throwing an error at that point.
>
> There should be no user visible changes after this commit.
> ---
> gdb/breakpoint.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index c629d27c4c0..c1440a6921f 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -8704,19 +8704,17 @@ code_breakpoint::code_breakpoint (struct gdbarch *gdbarch_,
> /* Do not set breakpoint locations conditions yet. As locations
> are inserted, they get sorted based on their addresses. Let
> the list stabilize to have reliable location numbers. */
> + }
>
> - /* Dynamic printf requires and uses additional arguments on the
> - command line, otherwise it's an error. */
> - if (type == bp_dprintf)
> - {
> - if (extra_string != nullptr)
> - update_dprintf_command_list (this);
> - else
> - error (_("Format string required"));
> - }
> - else if (extra_string != nullptr)
> - error (_("Garbage '%s' at end of command"), extra_string.get ());
> + /* Dynamic printf requires and uses additional arguments on the
> + command line, otherwise it's an error. */
> + if (type == bp_dprintf)
> + {
> + gdb_assert (extra_string != nullptr);
> + update_dprintf_command_list (this);

In Patch 3, a gdb_assert was added inside update_dprintf_command_list. I also
noticed that in dprintf_breakpoint::re_set, we have

/* extra_string should never be non-NULL for dprintf. */
gdb_assert (extra_string != NULL);

/* 1 - connect to target 1, that can run breakpoint commands.
2 - create a dprintf, which resolves fine.
3 - disconnect from target 1
4 - connect to target 2, that can NOT run breakpoint commands.

After steps #3/#4, you'll want the dprintf command list to
be updated, because target 1 and 2 may well return different
answers for target_can_run_breakpoint_commands().
Given absence of finer grained resetting, we get to do
it all the time. */
if (extra_string != NULL)
update_dprintf_command_list (this);

So, I wonder if it wouldn't be simpler to have the assert in
update_dprintf_command_list only and remove the asserts and if-checks
from the call sites.

Thanks
-Baris

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928