This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA v4] enable/disable sub breakpoint range


Hi Xavier,

On 10/23/2017 12:07 PM, Xavier Roirand wrote:

> You're right, I was not using the right command to produce the patch and
> it did not work on my side either :( I've generated a new version below
> and tested it successfully without the previous issue. In case of
> copy/paste issue, I've attached it as well.
> Thx

Thanks.  I got it, and have been playing with it.

I noticed a few things that would need fixing.  I started playing
with one of the suggestions I'd be giving to see if it'd really
work (get_number -> get_number_trailer), and ended up fixing the
other issues I found too.

Looks like you didn't address Eli's comments on the manual parts
between v3 -> v4.  I've done that too.

At the bottom I've pasted a diff of my changes on top of your patch.
I'll squash that in and post a v5 in a moment.

Below I've pointed out some of the issues I found, FYI and for
reference.

Thanks again for the patch.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index fbf5591..76d7cec 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -75,6 +75,11 @@ QSetWorkingDir
>  * The "maintenance selftest" command now takes an optional argument to
>    filter the tests to be run.
> 
> +* Breakpoint commands accept location ranges.
> +
> +The breakpoint commands ``enable'', and ``disable'' now accept a
> +location range of breakpoints, e.g. ``1.3-5''.

I think we should say "range of breakpoint locations".  Here
and throughout.

> +
>  * New commands
> 
>  set|show cwd
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 32ceea7..c5a84eb 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -93,9 +93,18 @@ enum exception_event_kind
> 
>  /* Prototypes for local functions.  */
> 
> +static void disable_command (char *, int);
> +
> +static void enable_command (char *, int);
> +
> +static void map_breakpoint_number_range (std::pair <int, int>
> &bp_num_range,
> +                                         gdb::function_view<void
> (breakpoint *)>);
> +

These forward declarations are unnecessary.

>  static void map_breakpoint_numbers (const char *,
>                      gdb::function_view<void (breakpoint *)>);
> 
> +static void enable_disable_command (char *args, int from_tty, bool
> enable);
> +

And so is this one.  These new command-related functions actually
have the wrong prototype for master.  Should take "const char *"
instead of "char *".

>  static void ignore_command (char *, int);
> 
>  static void breakpoint_re_set_default (struct breakpoint *);
> @@ -14160,17 +14169,51 @@ ignore_command (char *args, int from_tty)
>    if (from_tty)
>      printf_filtered ("\n");
>  }
> -
> +
> +/* Call FUNCTION on each of the breakpoints present in range defined by
> +   BP_NUM_RANGE as pair of integer in which BP_NUM_RANGE.FIRST is the
> start
> +   of the breakpoint number range and BP_NUM_RANGE.SECOND is the end of
> +   the breakpoint number range.
> +   If BP_NUM_RANGE.FIRST == BP_NUM_RANGE.SECOND then the
> +   range is just a single breakpoint number.  */
> +
> +static void
> +map_breakpoint_number_range (std::pair <int, int> &bp_num_range,
> +                             gdb::function_view<void (breakpoint *)>
> function)
> +{
> +  if (bp_num_range.first == 0)
> +    {
> +      warning (_("bad breakpoint number at or near '%d'"),
> +               bp_num_range.first);
> +    }
> +  else
> +    {
> +      struct breakpoint *b, *tmp;
> +
> +      for (int i = bp_num_range.first; i <= bp_num_range.second; i++)
> +        {
> +          bool match = false;
> +
> +          ALL_BREAKPOINTS_SAFE (b, tmp)
> +            if (b->number == i)
> +              {
> +                match = true;
> +                function (b);
> +                break;
> +            }
> +          if (!match)
> +            printf_unfiltered (_("No breakpoint number %d.\n"), i);
> +        }
> +    }
> +}

There's a lot of tab/space mixup in your patch.  Please
configure your editor to preserve tabs.  I've fixed it all
locally.


> +/* Return the breakpoint location structure corresponding to the
> +   BP_NUM and LOC_NUM values.  */
> +
>  static struct bp_location *
> -find_location_by_number (const char *number)
> +find_location_by_number (int bp_num, int loc_num)
>  {
> -  const char *p1;
> -  int bp_num;
> -  int loc_num;
>    struct breakpoint *b;
>    struct bp_location *loc;
> 
> -  p1 = number;
> -  bp_num = get_number_trailer (&p1, '.');
> -  if (bp_num == 0 || p1[0] != '.')
> -    error (_("Bad breakpoint number '%s'"), number);
> -
>    ALL_BREAKPOINTS (b)
>      if (b->number == bp_num)
>        {
> @@ -14222,25 +14244,154 @@ find_location_by_number (const char *number)
>        }
> 
>    if (!b || b->number != bp_num)
> -    error (_("Bad breakpoint number '%s'"), number);
> +    error (_("Bad breakpoint number '%d'"), bp_num);
> 
> -  /* Skip the dot.  */
> -  ++p1;
> -  const char *save = p1;
> -  loc_num = get_number (&p1);
>    if (loc_num == 0)
> -    error (_("Bad breakpoint location number '%s'"), number);
> +    error (_("Bad breakpoint location number '%d'"), loc_num);
> 
>    --loc_num;
>    loc = b->loc;
>    for (;loc_num && loc; --loc_num, loc = loc->next)
>      ;
>    if (!loc)
> -    error (_("Bad breakpoint location number '%s'"), save);
> +    error (_("Bad breakpoint location number '%d'"), loc_num);

This introduces a regression in the error string:

Before:
 (gdb) disable 1.2
 Bad breakpoint location number '2'
 (gdb) disable 1.3
 Bad breakpoint location number '3'

After:
 (gdb) disable 1.2
 Bad breakpoint location number '0'
 (gdb) disable 1.3
 Bad breakpoint location number '1'

In the quoted hunk above, note how loc_num was decremented
before and in the for loop.


The new test exposes this, testsuite/gdb.log shows:

 disable 2.3-5
 Bad breakpoint location number '0'
 (gdb) PASS: gdb.cp/locbprange.exp: disable location breakpoint range with max > existing

Note the '0'.  It was still a PASS because the test uses $decimal:

 +gdb_test "disable 2.3-5" "Bad breakpoint location number '$decimal'" \
 +    "disable location breakpoint range with max > existing"

Those '$decimal's should instead be replaced by the right numbers.

> 
>    return loc;
>  }
> 
> +/* Extract the breakpoint range defined by ARG. Return the start of
> +   the breakpoint range defined by BP_NUM_RANGE.FIRST and
> +   BP_LOC_RANGE.FIRST and the end of the breakpoint range defined by
> +   BP_NUM_RANGE.second and BP_LOC_RANGE.SECOND.
> +
> +   The range may be any of the following form:
> +
> +   x     where x is breakpoint number.
> +   x-y   where x and y are breakpoint numbers range.
> +   x.y   where x is breakpoint number and z a location number.
> +   x.y-z where x is breakpoint number and y and z a location number
> +         range.  */
> +
> +static int

This returns '1' on error and '0' on success.  I think it'd be more
natural if this returned bool/true/false.  I.e. reverse the logic.
(Though it seems strange that there's one code path that warns
while all others error.  I'll fix that as a follow up.)

> +extract_bp_number_and_location (const std::string &arg,
> +                                std::pair <int, int> &bp_num_range,
> +                                std::pair <int, int> &bp_loc_range)
> +{
> +  std::size_t dot = arg.find (".");
> +
> +  if (dot != std::string::npos)
> +    {
> +      /* Handle x.y and x.y-z cases.  */
> +      std::size_t dash;
> +      std::string bp_loc;
> +
> +      if (arg.length () == dot + 1 || dot == 0)
> +        error (_("bad breakpoint number at or near: '%s'"), arg.c_str ());
> +
> +      dash = arg.find ("-", dot + 1);
> +
> +      bp_loc = arg.substr (dot + 1);
> +      const char *ptbf = arg.substr (0, dot).c_str ();

This is undefined behavior.  The 'arg.substr (0, dot)'
parts returns a std::string by value (a deep copy of the
substring), and that std::string temporary is going to die at
the end of the expression (at the ';').  This means that that '.c_str()'
gives you a pointer to the internal raw string of the
temporary that is about to die.  I.e., 'ptbf' is dangling after
this statement.

You could avoid that by splitting the statement in two
parts:

      std::string sub = arg.substr (0, dot);
      const char *ptbf = sub.c_str ();

... which would make the 'ptbf' pointer valid for as long
as 'sub' is (as long as you don't modify the string).

However...

> +      bp_num_range.first = get_number(&ptbf);
> +      bp_num_range.second = bp_num_range.first;

... we can use get_number_trailer instead which completely
avoids the need for the string copies.

This occurs several times in this function.

> +
> +      if (bp_num_range.first == 0)
> +        error (_("Bad breakpoint number '%s'"), arg.c_str ());
> +
> +      if (dash != std::string::npos)
> +        {
> +          /* bp_loc is range (x-z).  */
> +          if (arg.length () == dash + 1)
> +            error (_("bad breakpoint number at or near: '%s'"),
> arg.c_str ());
> +          dash = bp_loc.find ("-");
> +          const char *ptlf = bp_loc.substr (0, dash).c_str ();
> +          bp_loc_range.first = get_number(&ptlf);
> +          const char *ptls= bp_loc.substr (dash + 1).c_str ();
> +          bp_loc_range.second = get_number(&ptls);

Missing space before '=' while at it.

> +        }
> +      else
> +        {
> +          /* bp_loc is single value.  */
> +          const char *ptls= bp_loc.c_str ();

Ditto.

> +          bp_loc_range.second = get_number(&ptls);

Missing space before '('.

> +          bp_loc_range.first = bp_loc_range.second;
> +          if (bp_loc_range.first == 0)
> +            {
> +              warning (_("bad breakpoint number at or near '%s'"),
> arg.c_str ());
> +              return 1;
> +            }
> +        }
> +    }
> +  else
> +    {
> +      /* Handle x and x-y cases.  */
> +      std::size_t dash;
> +
> +      dash = arg.find ("-");
> +      bp_loc_range.first = 0;
> +      bp_loc_range.second = 0;
> +      if (dash != std::string::npos)
> +        {
> +          if (arg.length () == dash + 1 || dash == 0)
> +            error (_("bad breakpoint number at or near: '%s'"),
> arg.c_str ());
> +
> +      const char *ptlf = arg.substr (0, dash).c_str ();
> +          bp_num_range.first = get_number (&ptlf);
> +          const char *ptls= arg.substr (dash + 1).c_str ();
> +          bp_num_range.second = get_number (&ptls);
> +        }
> +      else
> +        {
> +          const char * ptlf = arg.c_str ();
> +          bp_num_range.first = get_number (&ptlf);
> +          bp_num_range.second = bp_num_range.first;
> +          if (bp_num_range.first == 0)
> +            {
> +              warning (_("bad breakpoint number at or near '%s'"),
> arg.c_str ());
> +              return 1;
> +            }
> +        }
> +    }
> +
> +  if (bp_num_range.first == 0 || bp_num_range.second == 0)
> +    error (_("bad breakpoint number at or near: '%s'"), arg.c_str ());
> +
> +  return 0;
> +}
> +


> b/gdb/testsuite/gdb.cp/locbprange.cc
> new file mode 100644
> index 0000000..ff44b50
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/locbprange.cc
> @@ -0,0 +1,57 @@

> +
> +#include <stddef.h>
> +
> +class foo
> +  {
> +  public:
> +    static int overload (void);
> +    static int overload (char);
> +    static int overload (int);
> +    static int overload (double);
> +  };
> +
> +void marker1()
> +  {
> +  }
> +
> +int main ()
> +  {
> +    foo::overload ();
> +    foo::overload (111);
> +    foo::overload ('h');
> +    foo::overload (3.14);
> +
> +    marker1 (); // marker1-returns-here
> +
> +    return 0;
> +  }
> +
> +/* Some functions to test overloading by varying one argument type. */
> +
> +int foo::overload (void)
> +  {
> +    return 1;
> +  }
> +int foo::overload (char arg)
> +  {
> +    arg = 0;
> +    return 2;
> +  }
> +int foo::overload (int arg)             { arg = 0; return 3;}
> +int foo::overload (double arg)          { arg = 0; return 4;}

We follow GNU coding conventions/standards in the tests too
unless the test requires otherwise.  (We have old tests that predate
that rule, though.)


> diff --git a/gdb/testsuite/gdb.cp/locbprange.exp
> b/gdb/testsuite/gdb.cp/locbprange.exp
> new file mode 100644
> index 0000000..2a13791
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/locbprange.exp
> @@ -0,0 +1,160 @@
> +# Copyright 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/>.
> +
> +# This file is part of the gdb testsuite

Missing period.

> +
> +# Tests for breakpoint location range enable/disable commands.
> +
> +set ws "\[\r\n\t \]+"
> +set nl "\[\r\n\]+"

These are not used.

> +
> +
> +if { [skip_cplus_tests] } { continue }
> +
> +standard_testfile .cc
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug
> c++}]} {
> +    return -1
> +}
> +
> +# Set it up at a breakpoint so we can play with the variable values.

We're not playing with variables.

> +
> +if ![runto 'marker1'] then {
> +    perror "couldn't run to marker1"
> +    continue
> +}
> +
> +# Prevent symbol on address 0x0 being printed.
> +gdb_test_no_output "set print symbol off"

Not necessary for this testscase.

> +
> +gdb_test "up" ".*main.*" "up from marker1"

Not necessary either.

> +
> +# Returns a buffer corresponding to what gdb replies when
> +# asking for 'info breakpoint'. The parameters are all the
> +# existing breakpoints enabled/disable value: 'n' or 'y'.
> +
> +proc set_info_breakpoint_reply {b1 b2 b21 b22 b23 b24} {

I think I can see where the "set" comes from, though I think
"get_info..." or "make_info" instead of "set_info..." would
read a bit less surprising.

> +
> +    set buf "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
> +1\[\t \]+breakpoint     keep $b1.* in marker1\\(\\) at .*
> +\[\t \]+breakpoint already hit 1 time.*
> +2\[\t \]+breakpoint\[\t \]+keep $b2\[\t \]+<MULTIPLE>.*
> +2.1\[\t \]+$b21.*
> +2.2\[\t \]+$b22.*
> +2.3\[\t \]+$b23.*
> +2.4\[\t \]+$b24.*"

Use multi_line.  Can use something like the $ws variable here.

> +
> +    return $buf
> +}
> +
> +gdb_test "break foo::overload" \
> +    "Breakpoint \[0-9\]+ at $hex: foo::overload. .4 locations." \
> +    "set breakpoint at overload"
> +
> +gdb_test "info break" [set_info_breakpoint_reply y y y y y y] \
> +    "breakpoint info"
> +
> +# Check that we can disable a breakpoint.
> +gdb_test_no_output "disable 1"
> +
> +gdb_test "info break" [set_info_breakpoint_reply n y y y y y] \
> +    "breakpoint info disable bkpt 1"

This "disable" -> "check with 'info break'" pattern is repeated
several times.  We can move that to a helper procedure.

> +gdb_test "info break" [set_info_breakpoint_reply y y y y y y] \
> +    "breakpoint info enaable bkpt 2.2 to 2.3"

... which eliminates this "enabble" typo above.

> +# Check that disabling an reverse location breakpoint range does
> +# not work.
> +gdb_test_no_output "disable 2.3-2"

I think this should complain loudly instead of silently, but I'll
leave that for a follow up patch.

> +
> +gdb_test "info break" [set_info_breakpoint_reply y y y n y y] \
> +    "breakpoint info disable 2.3 to 2.3"
> +
> +# Check that disabling an unvalid location breakpoint range does
> +# not cause unexpected behavior.
> +gdb_test "disable 2.6-7" "Bad breakpoint location number '$decimal'" \
> +    "disable an unvalid location breakpoint range"
> +
> +gdb_test "info break" [set_info_breakpoint_reply y y y n y y] \
> +    "breakpoint info disable 2.6 to 2.7"
> +
> +# Check that disabling an invalid location breakpoint range does not
> +# cause trouble.
> +gdb_test_no_output "disable 2.8-6"

Ditto.

> +
> +gdb_test "info break" [set_info_breakpoint_reply y y y n y y] \
> +    "breakpoint info disable 2.8 to 2.6"

Follows the diff of my changes that apply on top of your
patch.  (This is a "git diff -w" because there were a good
number of whitespace fixes.)

diff --git c/gdb/doc/gdb.texinfo w/gdb/doc/gdb.texinfo
index b1346d9..29d4789 100644
--- c/gdb/doc/gdb.texinfo
+++ w/gdb/doc/gdb.texinfo
@@ -3927,20 +3927,15 @@ Num     Type           Disp Enb  Address    What
 1.2                         y    0x080486ca in void foo<double>() at t.cc:8
 @end smallexample
 
-Each location can be individually enabled or disabled by passing
+You cannot delete the individual locations from a breakpoint.  However,
+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.  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},
+@code{enable} and @code{disable} a range of @var{location-number}
+locations using a @var{breakpoint-number} and two @var{location-number}s,
 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).
+@kbd{@var{breakpoint-number}.@var{location-number1}-@var{location-number2}},
+in which case @value{GDBN} acts on all the locations in the range (inclusive).
 Disabling or enabling the parent breakpoint (@pxref{Disabling}) affects
 all of the locations that belong to that breakpoint.
 
diff --git c/gdb/NEWS w/gdb/NEWS
index 76d7cec..aadb7a3 100644
--- c/gdb/NEWS
+++ w/gdb/NEWS
@@ -75,10 +75,8 @@ QSetWorkingDir
 * The "maintenance selftest" command now takes an optional argument to
   filter the tests to be run.
 
-* Breakpoint commands accept location ranges.
-
-The breakpoint commands ``enable'', and ``disable'' now accept a
-location range of breakpoints, e.g. ``1.3-5''.
+* The "enable", and "disable" commands now accept a range of
+  breakpoint locations, e.g. "enable 1.3-5".
 
 * New commands
 
diff --git c/gdb/breakpoint.c w/gdb/breakpoint.c
index b0b16c7..2e43013 100644
--- c/gdb/breakpoint.c
+++ w/gdb/breakpoint.c
@@ -93,18 +93,9 @@ enum exception_event_kind
 
 /* Prototypes for local functions.  */
 
-static void disable_command (char *, int);
-
-static void enable_command (char *, int);
-
-static void map_breakpoint_number_range (std::pair <int, int> &bp_num_range,
-                                         gdb::function_view<void (breakpoint *)>);
-
 static void map_breakpoint_numbers (const char *,
 				    gdb::function_view<void (breakpoint *)>);
 
-static void enable_disable_command (char *args, int from_tty, bool enable);
-
 static void ignore_command (char *, int);
 
 static void breakpoint_re_set_default (struct breakpoint *);
@@ -14169,15 +14160,12 @@ ignore_command (char *args, int from_tty)
     printf_filtered ("\n");
 }
 
-/* Call FUNCTION on each of the breakpoints present in range defined by
-   BP_NUM_RANGE as pair of integer in which BP_NUM_RANGE.FIRST is the start
-   of the breakpoint number range and BP_NUM_RANGE.SECOND is the end of
-   the breakpoint number range.
-   If BP_NUM_RANGE.FIRST == BP_NUM_RANGE.SECOND then the
-   range is just a single breakpoint number.  */
+
+/* Call FUNCTION on each of the breakpoints with numbers in the range
+   defined by BP_NUM_RANGE (an inclusive range).  */
 
 static void
-map_breakpoint_number_range (std::pair <int, int> &bp_num_range,
+map_breakpoint_number_range (std::pair<int, int> bp_num_range,
 			     gdb::function_view<void (breakpoint *)> function)
 {
   if (bp_num_range.first == 0)
@@ -14206,14 +14194,14 @@ map_breakpoint_number_range (std::pair <int, int> &bp_num_range,
     }
 }
 
-/* Call FUNCTION on each of the breakpoints
-   whose numbers are given in ARGS.  */
+/* Call FUNCTION on each of the breakpoints whose numbers are given in
+   ARGS.  */
 
 static void
 map_breakpoint_numbers (const char *args,
 			gdb::function_view<void (breakpoint *)> function)
 {
-  if (args == 0 || *args == '\0')
+  if (args == NULL || *args == '\0')
     error_no_arg (_("one or more breakpoint numbers"));
 
   number_or_range_parser parser (args);
@@ -14221,9 +14209,7 @@ map_breakpoint_numbers (const char *args,
   while (!parser.finished ())
     {
       int num = parser.get_number ();
-      std::pair <int,int> range = std::make_pair (num, num);
-
-      map_breakpoint_number_range (range, function);
+      map_breakpoint_number_range (std::make_pair (num, num), function);
     }
 }
 
@@ -14234,7 +14220,6 @@ static struct bp_location *
 find_location_by_number (int bp_num, int loc_num)
 {
   struct breakpoint *b;
-  struct bp_location *loc;  
 
   ALL_BREAKPOINTS (b)
     if (b->number == bp_num)
@@ -14248,117 +14233,114 @@ find_location_by_number (int bp_num, int loc_num)
   if (loc_num == 0)
     error (_("Bad breakpoint location number '%d'"), loc_num);
 
-  --loc_num;
-  loc = b->loc;
-  for (;loc_num && loc; --loc_num, loc = loc->next)
-    ;
-  if (!loc)
-    error (_("Bad breakpoint location number '%d'"), loc_num);
-    
+  int n = 0;
+  for (bp_location *loc = b->loc; loc != NULL; loc = loc->next)
+    if (++n == loc_num)
       return loc;
+
+  error (_("Bad breakpoint location number '%d'"), loc_num);
 }
 
-/* Extract the breakpoint range defined by ARG. Return the start of
-   the breakpoint range defined by BP_NUM_RANGE.FIRST and
-   BP_LOC_RANGE.FIRST and the end of the breakpoint range defined by
-   BP_NUM_RANGE.second and BP_LOC_RANGE.SECOND.
+/* Extract the breakpoint/location range specified by ARG.  Returns
+   the breakpoint range in BP_NUM_RANGE, and the location range in
+   BP_LOC_RANGE.
 
-   The range may be any of the following form:
+   ARG may be in any of the following forms:
 
-   x     where x is breakpoint number.
-   x-y   where x and y are breakpoint numbers range.
-   x.y   where x is breakpoint number and z a location number.
-   x.y-z where x is breakpoint number and y and z a location number
-         range.  */
+   x     where 'x' is a breakpoint number.
+   x-y   where 'x' and 'y' specify a breakpoint numbers range.
+   x.y   where 'x' is a breakpoint number and 'z' a location number.
+   x.y-z where 'x' is a breakpoint number and 'y' and 'z' specify a
+	 location number range.
+*/
 
-static int
+static bool
 extract_bp_number_and_location (const std::string &arg,
 				std::pair<int, int> &bp_num_range,
 				std::pair<int, int> &bp_loc_range)
 {
-  std::size_t dot = arg.find (".");
+  std::string::size_type dot = arg.find ('.');
 
   if (dot != std::string::npos)
     {
-      /* Handle x.y and x.y-z cases.  */
-      std::size_t dash;
-      std::string bp_loc;
+      /* Handle 'x.y' and 'x.y-z' cases.  */
 
       if (arg.length () == dot + 1 || dot == 0)
 	error (_("bad breakpoint number at or near: '%s'"), arg.c_str ());
 
-      dash = arg.find ("-", dot + 1);
-
-      bp_loc = arg.substr (dot + 1);
-      const char *ptbf = arg.substr (0, dot).c_str ();
-      bp_num_range.first = get_number(&ptbf);
-      bp_num_range.second = bp_num_range.first;
-
-      if (bp_num_range.first == 0)
+      const char *ptb = arg.c_str ();
+      int bp_num = get_number_trailer (&ptb, '.');
+      if (bp_num == 0)
 	error (_("Bad breakpoint number '%s'"), arg.c_str ());
 
+      bp_num_range.first = bp_num;
+      bp_num_range.second = bp_num;
+
+      const char *bp_loc = &arg[dot + 1];
+      std::string::size_type dash = arg.find ('-', dot + 1);
       if (dash != std::string::npos)
 	{
-          /* bp_loc is range (x-z).  */
+	  /* bp_loc is a range (x-z).  */
 	  if (arg.length () == dash + 1)
-            error (_("bad breakpoint number at or near: '%s'"), arg.c_str ());
-          dash = bp_loc.find ("-");
-          const char *ptlf = bp_loc.substr (0, dash).c_str ();
-          bp_loc_range.first = get_number(&ptlf);
-          const char *ptls= bp_loc.substr (dash + 1).c_str ();
-          bp_loc_range.second = get_number(&ptls);
+	    error (_("bad breakpoint number at or near: '%s'"), bp_loc);
+
+	  const char *ptlf = bp_loc;
+	  bp_loc_range.first = get_number_trailer (&ptlf, '-');
+
+	  const char *ptls = &arg[dash + 1];
+	  bp_loc_range.second = get_number_trailer (&ptls, '\0');
 	}
       else
 	{
-          /* bp_loc is single value.  */
-          const char *ptls= bp_loc.c_str ();
-          bp_loc_range.second = get_number(&ptls);
-          bp_loc_range.first = bp_loc_range.second;
+	  /* bp_loc is a single value.  */
+	  const char *ptls = bp_loc;
+	  bp_loc_range.first = get_number_trailer (&ptls, '\0');
 	  if (bp_loc_range.first == 0)
 	    {
 	      warning (_("bad breakpoint number at or near '%s'"), arg.c_str ());
-              return 1;
+	      return false;
 	    }
+	  bp_loc_range.second = bp_loc_range.first;
 	}
     }
   else
     {
       /* Handle x and x-y cases.  */
-      std::size_t dash;
-
-      dash = arg.find ("-");
-      bp_loc_range.first = 0;
-      bp_loc_range.second = 0;
+      std::string::size_type dash = arg.find ('-');
       if (dash != std::string::npos)
 	{
 	  if (arg.length () == dash + 1 || dash == 0)
 	    error (_("bad breakpoint number at or near: '%s'"), arg.c_str ());
 
-	  const char *ptlf = arg.substr (0, dash).c_str ();
-          bp_num_range.first = get_number (&ptlf);
-          const char *ptls= arg.substr (dash + 1).c_str ();
-          bp_num_range.second = get_number (&ptls);
+	  const char *ptf = arg.c_str ();
+	  bp_num_range.first = get_number_trailer (&ptf, '-');
+
+	  const char *pts = &arg[dash + 1];
+	  bp_num_range.second = get_number_trailer (&pts, '\0');
 	}
       else
 	{
-          const char * ptlf = arg.c_str ();
-          bp_num_range.first = get_number (&ptlf);
-          bp_num_range.second = bp_num_range.first;
+	  const char *ptf = arg.c_str ();
+	  bp_num_range.first = get_number (&ptf);
 	  if (bp_num_range.first == 0)
 	    {
 	      warning (_("bad breakpoint number at or near '%s'"), arg.c_str ());
-              return 1;
+	      return false;
 	    }
+	  bp_num_range.second = bp_num_range.first;
 	}
+      bp_loc_range.first = 0;
+      bp_loc_range.second = 0;
     }
 
   if (bp_num_range.first == 0 || bp_num_range.second == 0)
     error (_("bad breakpoint number at or near: '%s'"), arg.c_str ());
 
-  return 0;
+  return true;
 }
 
-/* Enable or disable a breakpoint using BP_NUMB, LOC_NUM and enable.  */
+/* Enable or disable a breakpoint location BP_NUM.LOC_NUM.  ENABLE
+   specifies whether to enable or disable.  */
 
 static void
 enable_disable_bp_num_loc (int bp_num, int loc_num, bool enable)
@@ -14379,9 +14361,11 @@ enable_disable_bp_num_loc (int bp_num, int loc_num, bool enable)
   update_global_location_list (UGLL_DONT_INSERT);
 }
 
-/* Enable or disable a breakpoint location range.  It uses BP_NUM,
-   BP_LOC_RANGE.FIRST for the start of the range, BP_LOC_RANGE.SECOND for
-   the end of the range and enable.  */
+/* Enable or disable a range of breakpoint locations.  BP_NUM is the
+   number of the breakpoint, and BP_LOC_RANGE specifies the
+   (inclusive) range of location numbers of that breakpoint to
+   enable/disable.  ENABLE specifies whether to enable or disable the
+   location.  */
 
 static void
 enable_disable_breakpoint_location_range (int bp_num,
@@ -14424,12 +14408,13 @@ disable_breakpoint (struct breakpoint *bpt)
   observer_notify_breakpoint_modified (bpt);
 }
 
-/* Enable or disable breakpoint defined in ARGS.  Breakpoint may be
-   any of the form defined in extract_bp_number_and_location.
-   ENABLE enable or disable breakpoint.  */
+/* Enable or disable the breakpoint(s) or breakpoint location(s)
+   specified in ARGS.  ARGS may be in any of the formats handled by
+   extract_bp_number_and_location.  ENABLE specifies whether to enable
+   or disable the breakpoints/locations.  */
 
 static void
-enable_disable_command (char *args, int from_tty, bool enable)
+enable_disable_command (const char *args, int from_tty, bool enable)
 {
   if (args == 0)
     {
@@ -14450,25 +14435,23 @@ enable_disable_command (char *args, int from_tty, bool enable)
 
       while (!num.empty ())
 	{
-          std::pair <int, int> bp_num_range;
-          std::pair <int, int> bp_loc_range;
+	  std::pair<int, int> bp_num_range, bp_loc_range;
 
-          int err_ret = extract_bp_number_and_location (num.c_str(),
-                                                        bp_num_range,
-                                                        bp_loc_range);
-          if (!err_ret)
+	  if (extract_bp_number_and_location (num, bp_num_range, bp_loc_range))
 	    {
 	      if (bp_loc_range.first == bp_loc_range.second
 		  && bp_loc_range.first == 0)
 		{
-                  /* Handle breakpoint with format x or x-z only.  */
+		  /* Handle breakpoint ids with formats 'x' or 'x-z'.  */
 		  map_breakpoint_number_range (bp_num_range,
-                                               enable ? enable_breakpoint :
-                                                 disable_breakpoint);
+					       enable
+					       ? enable_breakpoint
+					       : disable_breakpoint);
 		}
 	      else
 		{
-                  /* Handle breakpoint with format is x.y or x.y-z */
+		  /* Handle breakpoint ids with formats 'x.y' or
+		     'x.y-z'.  */
 		  enable_disable_breakpoint_location_range
 		    (bp_num_range.first, bp_loc_range, enable);
 		}
@@ -14478,13 +14461,13 @@ enable_disable_command (char *args, int from_tty, bool enable)
     }
 }
 
-/* The disable command disables the specified breakpoints (or all defined
-   breakpoints) so they once stop be effective in stopping
-   the inferior.  ARGS may be any of the form defined in
+/* The disable command disables the specified breakpoints/locations
+   (or all defined breakpoints) so they're no longer effective in
+   stopping the inferior.  ARGS may be in any of the forms defined in
    extract_bp_number_and_location.  */
 
 static void
-disable_command (char *args, int from_tty)
+disable_command (const char *args, int from_tty)
 {
   enable_disable_command (args, from_tty, false);
 }
@@ -14559,10 +14542,10 @@ enable_breakpoint (struct breakpoint *bpt)
   enable_breakpoint_disp (bpt, bpt->disposition, 0);
 }
 
-/* The enable command enables the specified breakpoints (or all defined
-   breakpoints) so they once again become (or continue to be) effective
-   in stopping the inferior.  ARGS may be any of the form defined in
-   extract_bp_number_and_location.  */
+/* The enable command enables the specified breakpoints/locations (or
+   all defined breakpoints) so they once again become (or continue to
+   be) effective in stopping the inferior.  ARGS may be in any of the
+   forms defined in extract_bp_number_and_location.  */
 
 static void
 enable_command (const char *args, int from_tty)
diff --git c/gdb/testsuite/gdb.cp/locbprange.cc w/gdb/testsuite/gdb.cp/locbprange.cc
index ff44b50..caae259 100644
--- c/gdb/testsuite/gdb.cp/locbprange.cc
+++ w/gdb/testsuite/gdb.cp/locbprange.cc
@@ -20,38 +20,49 @@
 class foo
 {
 public:
-    static int overload (void);
-    static int overload (char);
-    static int overload (int);
-    static int overload (double);
+  static void overload (void);
+  static void overload (char);
+  static void overload (int);
+  static void overload (double);
 };
 
-void marker1()
+void
+marker ()
 {
 }
 
-int main ()
+int
+main ()
 {
   foo::overload ();
   foo::overload (111);
   foo::overload ('h');
   foo::overload (3.14);
 
-    marker1 (); // marker1-returns-here
+  marker ();
 
   return 0;
 }
 
-/* Some functions to test overloading by varying one argument type. */
+/* Some functions to test overloading by varying one argument
+   type.  */
 
-int foo::overload (void)
+void
+foo::overload ()
 {
-    return 1;
 }
-int foo::overload (char arg)
+
+void
+foo::overload (char arg)
+{
+}
+
+void
+foo::overload (int arg)
+{
+}
+
+void
+foo::overload (double arg)
 {
-    arg = 0;
-    return 2;
 }
-int foo::overload (int arg)             { arg = 0; return 3;}
-int foo::overload (double arg)          { arg = 0; return 4;}
diff --git c/gdb/testsuite/gdb.cp/locbprange.exp w/gdb/testsuite/gdb.cp/locbprange.exp
index 2a13791..0631b9f 100644
--- c/gdb/testsuite/gdb.cp/locbprange.exp
+++ w/gdb/testsuite/gdb.cp/locbprange.exp
@@ -13,13 +13,9 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-# This file is part of the gdb testsuite
-
-# Tests for breakpoint location range enable/disable commands.
-
-set ws "\[\r\n\t \]+"
-set nl "\[\r\n\]+"
+# This file is part of the gdb testsuite.
 
+# Test the enable/disable commands with breakpoint location ranges.
 
 if { [skip_cplus_tests] } { continue }
 
@@ -31,130 +27,104 @@ if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
 
 # Set it up at a breakpoint so we can play with the variable values.
 
-if ![runto 'marker1'] then {
-    perror "couldn't run to marker1"
-    continue
+if ![runto 'marker'] then {
+    fail "run to marker"
+    return -1
 }
 
-# Prevent symbol on address 0x0 being printed.
-gdb_test_no_output "set print symbol off"
-
-gdb_test "up" ".*main.*" "up from marker1"
-
-# Returns a buffer corresponding to what gdb replies when
-# asking for 'info breakpoint'. The parameters are all the
-# existing breakpoints enabled/disable value: 'n' or 'y'.
-
-proc set_info_breakpoint_reply {b1 b2 b21 b22 b23 b24} {
-
-    set buf "Num     Type\[ \]+Disp Enb Address\[ \]+What.*
-1\[\t \]+breakpoint     keep $b1.* in marker1\\(\\) at .*
-\[\t \]+breakpoint already hit 1 time.*
-2\[\t \]+breakpoint\[\t \]+keep $b2\[\t \]+<MULTIPLE>.*
-2.1\[\t \]+$b21.*
-2.2\[\t \]+$b22.*
-2.3\[\t \]+$b23.*
-2.4\[\t \]+$b24.*"
-
-    return $buf
+# Returns a buffer corresponding to what GDB replies when asking for
+# 'info breakpoint'.  The parameters are all the existing breakpoints
+# enabled/disable value: 'n' or 'y'.
+
+proc make_info_breakpoint_reply_re {b1 b2 b21 b22 b23 b24} {
+    set ws "\[\t \]+"
+    return [multi_line \
+		"Num     Type${ws}Disp Enb Address${ws}What.*" \
+		"1${ws}breakpoint     keep ${b1}${ws}.* in marker\\(\\) at .*" \
+		"${ws}breakpoint already hit 1 time.*" \
+		"2${ws}breakpoint${ws}keep${ws}${b2}${ws}<MULTIPLE>.*" \
+		"2.1${ws}${b21}.*" \
+		"2.2${ws}${b22}.*" \
+		"2.3${ws}${b23}.*" \
+		"2.4${ws}${b24}.*" \
+	       ]
 }
 
 gdb_test "break foo::overload" \
     "Breakpoint \[0-9\]+ at $hex: foo::overload. .4 locations." \
     "set breakpoint at overload"
 
-gdb_test "info break" [set_info_breakpoint_reply y y y y y y] \
+gdb_test "info break" [make_info_breakpoint_reply_re y y y y y y] \
     "breakpoint info"
 
-# Check that we can disable a breakpoint.
-gdb_test_no_output "disable 1"
-
-gdb_test "info break" [set_info_breakpoint_reply n y y y y y] \
-    "breakpoint info disable bkpt 1"
-
-# Check that we can enable a breakpoint.
-gdb_test_no_output "enable 1"
+# Test the enable/disable commands, and check the enable/disable state
+# of the breakpoints/locations in the "info break" output.  CMD is the
+# actual disable/enable command. The bNN parameters are the same as
+# make_info_breakpoint_reply_re's.
+proc test_enable_disable {cmd b1 b2 b21 b22 b23 b24} {
+    gdb_test_no_output $cmd
 
-gdb_test "info break" [set_info_breakpoint_reply y y y y y y] \
-    "breakpoint info enable bkpt 1"
-
-# Check that we can disable a breakpoint containing location breakpoints.
-gdb_test_no_output "disable 2"
-
-gdb_test "info break" [set_info_breakpoint_reply y n y y y y] \
-    "breakpoint info disable bkpt 2"
-
-# Check that we can enable a breakpoint containing location breakpoints.
-gdb_test_no_output "enable 2"
-
-gdb_test "info break" [set_info_breakpoint_reply y y y y y y] \
-    "breakpoint info enable bkpt 2"
-
-# Check that we can disable a single location breakpoint.
-gdb_test_no_output "disable 2.2"
-
-gdb_test "info break" [set_info_breakpoint_reply y y y n y y] \
-    "breakpoint info disable bkpt 2.2"
-
-# Check that we can enable a single location breakpoint.
-gdb_test_no_output "enable 2.2"
-
-gdb_test "info break" [set_info_breakpoint_reply y y y y y y] \
-    "breakpoint info enable bkpt 2.2"
-
-# Check that we can disable a location breakpoint range.
-gdb_test_no_output "disable 2.2-3"
+    set re [make_info_breakpoint_reply_re $b1 $b2 $b21 $b22 $b23 $b24]
+    gdb_test "info break" $re "breakpoint info $cmd"
+}
 
-gdb_test "info break" [set_info_breakpoint_reply y y y n n y] \
-    "breakpoint info disable bkpt 2.2 to 2.3"
+# Check that we can disable/enable a breakpoint with a single
+# location.
+test_enable_disable "disable 1" n y y y y y
+test_enable_disable "enable  1" y y y y y y
 
-# Check that we can enable a location breakpoint range.
-gdb_test_no_output "enable 2.2-3"
+# Check that we can disable/disable a breakpoint with multiple
+# locations.
+test_enable_disable "disable 2" y n y y y y
+test_enable_disable "enable  2" y y y y y y
 
-gdb_test "info break" [set_info_breakpoint_reply y y y y y y] \
-    "breakpoint info enaable bkpt 2.2 to 2.3"
+# Check that we can disable/enable a single location breakpoint.
+test_enable_disable "disable 2.2" y y y n y y
+test_enable_disable "enable  2.2" y y y y y y
 
-# Check that we can disable a location breakpoint range reduced
-# to a single location.
-gdb_test_no_output "disable 2.2-2"
+# Check that we can disable/enable a range of breakpoint locations.
+test_enable_disable "disable 2.2-3" y y y n n y
+test_enable_disable "enable  2.2-3" y y y y y y
 
-gdb_test "info break" [set_info_breakpoint_reply y y y n y y] \
-    "breakpoint info disable 2.2 to 2.2"
+# Check that we can disable/enable a breakpoint location range with
+# START==END.
+test_enable_disable "disable 2.2-2" y y y n y y
+test_enable_disable "enable  2.2-2" y y y y y y
 
 # Check that we can disable a location breakpoint range with max >
 # existing breakpoint location.
-gdb_test "disable 2.3-5" "Bad breakpoint location number '$decimal'" \
+gdb_test "disable 2.3-5" "Bad breakpoint location number '5'" \
     "disable location breakpoint range with max > existing"
 
-gdb_test "info break" [set_info_breakpoint_reply y y y n n n] \
+gdb_test "info break" [make_info_breakpoint_reply_re y y y y n n] \
     "breakpoint info disable 2.3 to 2.5"
 
 # Check that we can enable a location breakpoint range with max >
 # existing breakpoint location.
-gdb_test "enable 2.3-5" "Bad breakpoint location number '$decimal'" \
+gdb_test "enable 2.3-5" "Bad breakpoint location number '5'" \
     "enable location breakpoint range with max > existing"
 
-gdb_test "info break" [set_info_breakpoint_reply y y y n y y] \
+gdb_test "info break" [make_info_breakpoint_reply_re y y y y y y] \
     "breakpoint info enable 2.3 to 2.5"
 
-# Check that disabling an reverse location breakpoint range does
-# not work.
+# Check that disabling an reverse location breakpoint range does not
+# work.
 gdb_test_no_output "disable 2.3-2"
 
-gdb_test "info break" [set_info_breakpoint_reply y y y n y y] \
-    "breakpoint info disable 2.3 to 2.3"
+gdb_test "info break" [make_info_breakpoint_reply_re y y y y y y] \
+    "breakpoint info disable 2.3-2"
 
-# Check that disabling an unvalid location breakpoint range does
-# not cause unexpected behavior.
-gdb_test "disable 2.6-7" "Bad breakpoint location number '$decimal'" \
+# Check that disabling an invalid breakpoint location range does not
+# cause unexpected behavior.
+gdb_test "disable 2.6-7" "Bad breakpoint location number '6'" \
     "disable an unvalid location breakpoint range"
 
-gdb_test "info break" [set_info_breakpoint_reply y y y n y y] \
-    "breakpoint info disable 2.6 to 2.7"
+gdb_test "info break" [make_info_breakpoint_reply_re y y y y y y] \
+    "breakpoint info disable 2.6-7"
 
-# Check that disabling an invalid location breakpoint range does not
+# Check that disabling an invalid breakpoint location range does not
 # cause trouble.
 gdb_test_no_output "disable 2.8-6"
 
-gdb_test "info break" [set_info_breakpoint_reply y y y n y y] \
-    "breakpoint info disable 2.8 to 2.6"
+gdb_test "info break" [make_info_breakpoint_reply_re y y y y y y] \
+    "breakpoint info disable 2.8-6"


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]