diff mbox series

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

Message ID 3c240e1430759e5bdf78207106d79207a92c5f44.1701513409.git.aburgess@redhat.com
State New
Headers
Series [PATCHv6,01/10] gdb: create_breakpoint: add asserts and additional comments |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Andrew Burgess Dec. 2, 2023, 10:42 a.m. UTC 
  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(-)

Comments

Aktemur, Tankut Baris Dec. 5, 2023, 8:17 a.m. UTC | #1 
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
diff mbox series

Patch

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);
     }
+  else if (extra_string != nullptr)
+    error (_("Garbage '%s' at end of command"), extra_string.get ());
 
   /* The order of the locations is now stable.  Set the location
      condition using the location's number.  */