[RFA] enable/disable sub breakpoint range

Message ID aa6ee4a4-fc17-55c4-c211-6c0c724971f8@adacore.com
State New, archived
Headers

Commit Message

Xavier Roirand Aug. 17, 2017, 7:12 a.m. UTC
  This patch add capability to enable/disable sub breakpoints using range 
notation.

In some cases, adding one breakpoint corresponds to multiple
places in a program thus leads GDB to defines main breakpoint
number and let's say "sub breakpoint" using location number
with syntax <breakpoint_number>.<location_number>.

For example:
   Num  Type       Disp Enb  Address    What
   1    breakpoint keep y    <MULTIPLE>
   1.1                  y    0x080486a2 in void foo<int>()...
   1.2                  y    0x080486ca in void foo<double>()...

In this example, main breakpoint is breakpoint 1 where sub
breakpoints are 1.1 and 1.2 ones.

This patch allows enable/disable a range of sub breakpoints
using syntax:

<breakpoint_number>.<first_location_number>-<last_location_number>

with inclusive last_location_number.

For instance, if adding a breakpoint to foo() generates 6 sub
breakpoints from 1.1 to 1.6 then it's now possible to enable/disable
only sub breakpoint 1.3 to sub breakpoint 1.5 (so 1.3, 1.4 and 1.5)
using syntax:

enable 1.3-5 or disable 1.3-5

gdb/ChangeLog:

         * breakpoint.c (enable_disable_bp_with_num): create from
         enable/disable command refactoring.
         (enable_disable_sub_bp_range): create for handling sub
         breakpoint range parsing.
         (disable_command): refactor using enable_disable_bp_with_num
         now.
         (enable_command): refactor using enable_disable_bp_with_num now.

gdb/doc/ChangeLog:

         * gdb.texinfo (Set Breaks): add documentation for sub breakpoint
         range enable/disable action.
  

Comments

Joel Brobecker Aug. 17, 2017, 9:38 p.m. UTC | #1
> This patch add capability to enable/disable sub breakpoints using range
> notation.
[...]
> gdb/ChangeLog:
> 
>         * breakpoint.c (enable_disable_bp_with_num): create from
>         enable/disable command refactoring.
>         (enable_disable_sub_bp_range): create for handling sub
>         breakpoint range parsing.
>         (disable_command): refactor using enable_disable_bp_with_num
>         now.
>         (enable_command): refactor using enable_disable_bp_with_num now.
> 
> gdb/doc/ChangeLog:
> 
>         * gdb.texinfo (Set Breaks): add documentation for sub breakpoint
>         range enable/disable action.

For the record, Xavier also submitted this patch internally, and
I provided some comments which I think Xavier will try to address
with a second version of the patch.

In particular, one of my comments is that it seems that we are going
back and forth between strings and numbers just because some of
the function we re-use here take a string as a paraeeter when
conceptually the parameter should be a number. I think we can avoid
that unnecessary back-and-forth by splitting the string parsing from
the actual handling of those numbers.

One minor comment is that I think we should avoid modifying the input
string.
  

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 3dc9112..3f66b76 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14628,6 +14628,67 @@  find_location_by_number (char *number)
    return loc;
  }

+/* Enable or disable a breakpoint. Variable num has form:
+   <breakpoint_number>.<location_number>  */
+
+static void
+enable_disable_bp_with_num (char *num, int enable)
+{
+  struct bp_location *loc = find_location_by_number (num);
+
+  if (loc != NULL)
+    {
+      if (loc->enabled != enable)
+        {
+          loc->enabled = enable;
+          mark_breakpoint_location_modified (loc);
+        }
+      if (target_supports_enable_disable_tracepoint ()
+          && current_trace_status ()->running && loc->owner
+          && is_tracepoint (loc->owner))
+        target_disable_tracepoint (loc);
+    }
+  update_global_location_list (UGLL_DONT_INSERT);
+}
+
+/* Enable or disable a sub breakpoint range. Variable
+   number is the sub breakpoint range using form:
+   <bkpt_number>.<start_location_number>-<end_location_number>.  */
+
+static void
+enable_disable_sub_bp_range (char *number, int enable)
+{
+  char *dot = strchr (number, '.');
+  if (dot && dot[1] != '\0')
+    {
+      char *p1;
+      int bp_num;
+
+      p1 = number;
+      *dot = '\0';
+      bp_num = get_number (&p1);
+
+      /* Restore original string value.  */
+      *dot = '.';
+
+      /* Initialize the range parser.  */
+      number_or_range_parser parser (dot+1);
+
+      /* Iterate all the numbers in the range.  */
+      while (!parser.finished ())
+        {
+          char num_str[255];
+          int num;
+
+          num = parser.get_number ();
+
+          /* Build breakpoint name using following syntax:
+             <breakpoint_number>.<location_number>  */
+          snprintf (num_str, sizeof (num_str), "%d.%d", bp_num, num);
+          enable_disable_bp_with_num (num_str, enable);
+        }
+    }
+}

  /* Set ignore-count of breakpoint number BPTNUM to COUNT.
     If from_tty is nonzero, it prints a message to that effect,
@@ -14694,29 +14755,16 @@  disable_command (char *args, int from_tty)
        char *num = extract_arg (&args);

        while (num)
-	{
-	  if (strchr (num, '.'))
-	    {
-	      struct bp_location *loc = find_location_by_number (num);
-
-	      if (loc)
-		{
-		  if (loc->enabled)
-		    {
-		      loc->enabled = 0;
-		      mark_breakpoint_location_modified (loc);
-		    }
-		  if (target_supports_enable_disable_tracepoint ()
-		      && current_trace_status ()->running && loc->owner
-		      && is_tracepoint (loc->owner))
-		    target_disable_tracepoint (loc);
-		}
-	      update_global_location_list (UGLL_DONT_INSERT);
-	    }
-	  else
-	    map_breakpoint_numbers (num, do_map_disable_breakpoint, NULL);
-	  num = extract_arg (&args);
-	}
+        {
+          char *dash = strchr (num, '-');
+          if (dash)
+            enable_disable_sub_bp_range (num, 0);
+          else if (strchr (num, '.'))
+            enable_disable_bp_with_num (num, 0);
+          else
+            map_breakpoint_numbers (num, do_map_disable_breakpoint, NULL);
+          num = extract_arg (&args);
+        }
      }
  }

@@ -14825,29 +14873,16 @@  enable_command (char *args, int from_tty)
        char *num = extract_arg (&args);

        while (num)
-	{
-	  if (strchr (num, '.'))
-	    {
-	      struct bp_location *loc = find_location_by_number (num);
-
-	      if (loc)
-		{
-		  if (!loc->enabled)
-		    {
-		      loc->enabled = 1;
-		      mark_breakpoint_location_modified (loc);
-		    }
-		  if (target_supports_enable_disable_tracepoint ()
-		      && current_trace_status ()->running && loc->owner
-		      && is_tracepoint (loc->owner))
-		    target_enable_tracepoint (loc);
-		}
-	      update_global_location_list (UGLL_MAY_INSERT);
-	    }
-	  else
-	    map_breakpoint_numbers (num, do_map_enable_breakpoint, NULL);
-	  num = extract_arg (&args);
-	}
+        {
+          char *dash = strchr (num, '-');
+          if (dash)
+            enable_disable_sub_bp_range (num, 1);
+          else if (strchr (num, '.'))
+            enable_disable_bp_with_num (num, 1);
+          else
+            map_breakpoint_numbers (num, do_map_enable_breakpoint, NULL);
+          num = extract_arg (&args);
+        }
      }
  }

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 17b4c69..e96c318 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3876,13 +3876,20 @@  Num     Type           Disp Enb  Address    What

  Each location can be individually enabled or disabled by passing
  @var{breakpoint-number}.@var{location-number} as argument to the
-@code{enable} and @code{disable} commands.  Note that you cannot
-delete the individual locations from the list, you can only delete the
-entire list of locations that belong to their parent breakpoint (with
-the @kbd{delete @var{num}} command, where @var{num} is the number of
-the parent breakpoint, 1 in the above example).  Disabling or enabling
-the parent breakpoint (@pxref{Disabling}) affects all of the locations
-that belong to that breakpoint.
+@code{enable} and @code{disable} commands.  It's also possible to
+@code{enable} and @code{disable} range of @var{location-number}
+breakpoints using a @var{breakpoint-number} and two @var{location-number},
+in increasing order, separated by a hyphen, like
+‘@var{breakpoint-number}.5-7’.
+In this case, when a @var{location-number} range is given to this
+command, all breakpoints belonging to this @var{breakpoint-number}
+and inside that range are operated on.
+Note that you cannot delete the individual locations from the list, you
+can only delete the entire list of locations that belong to their parent
+breakpoint (with the @kbd{delete @var{num}} command, where @var{num} is
+the number of the parent breakpoint, 1 in the above example).  Disabling
+or enabling the parent breakpoint (@pxref{Disabling}) affects all of the
+locations that belong to that breakpoint.

  @cindex pending breakpoints
  It's quite common to have a breakpoint inside a shared library.
diff --git a/gdb/testsuite/gdb.ada/sub_bp_range.exp 
b/gdb/testsuite/gdb.ada/sub_bp_range.exp
new file mode 100644
index 0000000..ec6b784
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/sub_bp_range.exp
@@ -0,0 +1,45 @@ 
+# Copyright 2012-2017 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib "ada.exp"
+
+standard_ada_testfile foo
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable [list debug 
]] != "" } {
+  return -1
+}
+
+clean_restart ${testfile}
+
+gdb_test "break Raise_To_Power" \
+         "Breakpoint $decimal at $hex: Raise_To_Power. .$decimal 
locations."
+
+gdb_test "info b" \
+         "Num     Type\[ \]+Disp Enb Address\[ \]+What.*\[0-9\]+\[\t 
\]+breakpoint\[ \]+keep y.*\<MULTIPLE\>.*1\.1.* y .* in 
pck.raise_to_power.*1\.2.* y .* in pck.raise_to_power.*1\.3.* y .* in 
pck.raise_to_power.*1\.4.* y .* in pck.raise_to_power.*"
+
+gdb_test "disable 1.2-3" ""
+
+gdb_test "info b" \
+         "Num     Type\[ \]+Disp Enb Address\[ \]+What.*\[0-9\]+\[\t 
\]+breakpoint\[ \]+keep y.*\<MULTIPLE\>.*1\.1.* y .* in 
pck.raise_to_power.*1\.2.* n .* in pck.raise_to_power.*1\.3.* n .* in 
pck.raise_to_power.*1\.4.* y .* in pck.raise_to_power.*"
+
+gdb_test "disable 1.4-4" ""
+
+gdb_test "info b" \
+         "Num     Type\[ \]+Disp Enb Address\[ \]+What.*\[0-9\]+\[\t 
\]+breakpoint\[ \]+keep y.*\<MULTIPLE\>.*1\.1.* y .* in 
pck.raise_to_power.*1\.2.* n .* in pck.raise_to_power.*1\.3.* n .* in 
pck.raise_to_power.*1\.4.* n .* in pck.raise_to_power.*"
+
+gdb_test "enable 1.1-3" ""
+
+gdb_test "info b" \
+         "Num     Type\[ \]+Disp Enb Address\[ \]+What.*\[0-9\]+\[\t 
\]+breakpoint\[ \]+keep y.*\<MULTIPLE\>.*1\.1.* y .* in 
pck.raise_to_power.*1\.2.* y .* in pck.raise_to_power.*1\.3.* y .* in 
pck.raise_to_power.*1\.4.* n .* in pck.raise_to_power.*"
diff --git a/gdb/testsuite/gdb.ada/sub_bp_range/foo.adb 
b/gdb/testsuite/gdb.ada/sub_bp_range/foo.adb
new file mode 100644
index 0000000..755b46b
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/sub_bp_range/foo.adb
@@ -0,0 +1,22 @@ 
+--  Copyright 2012-2017 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+with Pck; use Pck;
+
+procedure Foo is
+   Small_Value : Integer := 10;
+begin
+   Do_Nothing (Small_Value'Address);  -- STOP
+end Foo;
diff --git a/gdb/testsuite/gdb.ada/sub_bp_range/pck.adb 
b/gdb/testsuite/gdb.ada/sub_bp_range/pck.adb
new file mode 100644
index 0000000..1dfa13d
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/sub_bp_range/pck.adb
@@ -0,0 +1,43 @@ 
+--  Copyright 2012-2017 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package body Pck is
+   procedure Do_Nothing (A : System.Address) is
+   begin
+      null;
+   end Do_Nothing;
+
+   function Raise_To_Power(Index : Integer) return Integer is
+   begin
+      return Index * Index;
+   end Raise_To_Power;
+
+   function Raise_To_Power(Value : Float) return Float is
+   begin
+      return Value * Value * Value;
+   end Raise_To_Power;
+
+   procedure Raise_To_Power(Index  : in  Integer;
+                            Result : out Integer) is
+   begin
+      Result := Index * Index * Index;
+   end Raise_To_Power;
+
+   procedure Raise_To_Power(Value  : in  Float;
+                            Result : out Float) is
+   begin
+      Result := Value * Value;
+   end Raise_To_Power;
+end Pck;
diff --git a/gdb/testsuite/gdb.ada/sub_bp_range/pck.ads 
b/gdb/testsuite/gdb.ada/sub_bp_range/pck.ads
new file mode 100644
index 0000000..74d4b39
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/sub_bp_range/pck.ads
@@ -0,0 +1,23 @@ 
+--  Copyright 2012-2017 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+with System;
+package Pck is
+   procedure Do_Nothing (A : System.Address);
+   function Raise_To_Power(Index : Integer) return Integer;
+   function Raise_To_Power(Value : Float) return Float;
+   procedure Raise_To_Power(Index: in Integer; Result: out Integer);
+   procedure Raise_To_Power(Value: in Float; Result: out Float);
+end Pck;