Re: [RFA] enable/disable sub breakpoint range

[Pedro Alves <palves at redhat dot com>]

Hi!

(I believe this is a v2.  It's nice to include that
in the email subject, like [PATCH v2]).

On 09/27/2017 03:49 PM, Xavier Roirand wrote:
> Add enable/disable sub breakpoint capability.

I've wanted this feature before!  Thanks for working on this.
We can already end up with disabled locations if a library is unloaded,
and we already try to propagate enabled/disabled status of locations
across breakpoint re-sets.  We were just missing exposing this
to users.

I'm not sure I like the inventing new terminology for this, though,
when we already call these "breakpoint location" throughout.
I.e., why not:

 [RFA] Allow enabling/disabling breakpoint locations

and then talk in terms of multiple locations throughout?

This isn't just a nit for the subject, because you're adding
references to the term to the tests at least.  I can't tell
whether you'd be adding them to the manual too, since it seems
like the manual hunk included in the patch is corrupt.  It's
mentioned in the manual's ChangeLog, though.

Some overall nits & comments:

 - Missing double-space after period in several comments throughout
   the patch.

 - Space-before ( missing in some cases throughout.

 - Tests don't follow GNU coding standards; but they should.

 - "written by" comments in the testcases are stale; in any case, we
   don't add "contributed by" comments to the sources anymore.

 - Missing copyright header in testcases.

 - Several missing full-stop periods in comments in the tests.

 - Are the Ada and C++ tests basically the same?  I wonder
   about maybe sharing most of the code both?  Maybe move
   the actual testing to a procedure called twice, once
   for each language.

> +
> +/* Call FUNC with data parameter on each of the breakpoints present in

s/FUNC/FUNCTION/  

Also there's no data parameter.  Maybe you had one before
function_view-fication.  Just remove that remark.


> +   range defined by BREAKPOINT_NUMBER_START => BREAKPOINT_NUMBER_END.
> +   If BREAKPOINT_NUMBER_START == BREAKPOINT_NUMBER_END. then the
> +   range is just a single breakpoint number.  */
> +
> +static void
> +map_breakpoint_number (int breakpoint_number_start,

I don't get the logic for the function name:

 map_breakpoint_number

Particularly since we have "map_breakpoint_numbers" (plural)
and also because this function works with a number range.

How about "map_breakpoint_number_range" instead?


> +                       int breakpoint_number_end,
> +                       gdb::function_view<void (breakpoint *)> function)
> +{
> +  int num;
> +  struct breakpoint *b, *tmp;

Please move these to the scope that requires them.

> +
> +  if (breakpoint_number_start == 0)
> +    {
> +      warning (_("bad breakpoint number at or near '%d'"),
> +               breakpoint_number_start);
> +    }
> +  else
> +    {
> +      int i;
> +
> +      for (i = breakpoint_number_start; i <= breakpoint_number_end; i++)

Similarly:

     for (int i = breakpoint_number_start

> +        {
> +          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);
> +        }
> +    }
> +}
> +
>  /* Call FUNCTION on each of the breakpoints
>     whose numbers are given in ARGS.  */

> +/* Return the breakpoint structure corresponding to the
> +   BP_NUM and LOC_NUM values.  */

The breakpoint _location_ structure.  The breakpoint structure
would be struct breakpoint.

> +
>  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)
>        {
> @@ -14379,25 +14406,152 @@ 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);
> 
>    return loc;
>  }
> 
> +/* Extract the breakpoint range defined by ARG and return the start of
> +   the breakpoint range defined by BP_NUM_START and BP_LOC_START and end
> +   of the breakpoint range defined by BP_NUM_END and BP_LOC_END.
> +   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.  */

Missing description of the return.

> +
> +static int
> +extract_bp_number_and_location (std::string arg, int *bp_num_start,
> +                                int *bp_num_end, int *bp_loc_start,
> +                                int *bp_loc_end)
> +{

The function does unnecessary string copying, starting with
passing ARG by value instead of const reference.

> +  char *tmp_str;
> +  std::string num = std::string (arg);

Copying a std::string results in a heap allocation and a
deep string copy.  There's need for this copy.  The body 
of the function has other similar issues (unnecessary xstrdup'ping),
but I can't go over all of them right now.  Please review the
function and try to eliminate all string copying.
Related, the function uses make_cleanup.  That's strange in
a function that is using std::string.  Please eliminate that.

As for the function's prototype, it may result in clearer
code to pass use a

 struct bp_range { int start; int end; };
or

 std::pair<int> 

for the ranges, like:

extract_bp_number_and_location (const std::string &arg,
                                std::pair<int, int> *bp_num_range,
                                std::pair<int, int> *bp_loc_range)


Also please add spaces around '+', and use /**/ instead of //.

> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 9905ff6..7699adb 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -3896,13 +3896,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.


This looks like a diff of a diff?

This should come with a NEWS entry too.

> +if {[gdb_compile_ada "${srcfile}" "${binfile}" executable {debug}] !=
> ""} {
> +    return -1
> +}
> +
> +# Returns a buffer corresponding to what gdb reply when

"replies"

> +# asking for 'info breakpoint'. The parameters are all the
> +# existing breakpoints enabled/disable value: 'n' or 'y'.
> +

Thanks,
Pedro Alves