Re: RFA: patch to fix multi-breakpoint enable/disable handling of inline functions

[Vladimir Prus <ghost@cs.msu.su>]

Douglas Evans wrote:

> Here's a patch.
> There is a heuristic involved in knowing when to not compare function
> names. I've tried to come up with something reasonable.

Thanks for the patch! I generally find it to be good incremental
improvement.

> 2007-10-15  Doug Evans  <dje@google.com>
> 
> * breakpoint.c (ambiguous_names_p): New fn.
> (update_breakpoint_locations): When restoring bp enable status, don't
> compare function names if all functions have same name.

IIUC, your patch avoids comparing function names if it finds
two locations having the same name, not necessary that *all* locations
have same name.

> 
> * gdb.cp/mb-inline.exp: New.
> * gdb.cp/mb-inline.h: New.
> * gdb.cp/mb-inline1.cc: New.
> * gdb.cp/mb-inline2.cc: New.
> 
> Index: breakpoint.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/breakpoint.c,v
> retrieving revision 1.274
> diff -u -u -p -r1.274 breakpoint.c
> --- ./breakpoint.c      12 Oct 2007 16:11:11 -0000      1.274
> +++ ./breakpoint.c      15 Oct 2007 23:47:54 -0000
> @@ -7496,6 +7496,37 @@ all_locations_are_pending (struct bp_loc
> return 1;
> }
> 
> +/* Return non-zero if multiple fns in list LOC have the same name.
> +   Null names are ignored.
> +   This returns zero if there's <= one named function, there's no
> ambiguity. +   ??? This tests for ambiguity with the first named function,
> it doesn't +   handle the case of multiple ambiguities.  This can be fixed
> at the cost of +   some complexity in the caller, but it's unknown if this
> is a practical +   issue.  */

I find the comment a bit confusing. How about this:

/* Return non-zero if any two locations in LOC have the 
   function_name field non-NULL and equal. Non-zero return means
   we cannot use function names to uniquely identify locations
   in this list.
   Although it's possible to identify groups of locations with
   the same name, this functions does not try to do that, as the
   code for matching old and new locations does not have use for
   such elaborate functionality.  */

> +
> +static int
> +ambiguous_names_p (struct bp_location *loc)
> +{
> +  struct bp_location *l;
> +  const char *name = NULL;
> +
> +  for (l = loc; l != NULL; l = l->next)
> +    {
> +      /* Allow for some names to be NULL, ignore them.  */
> +      if (l->function_name == NULL)
> +       continue;
> +      if (name == NULL)
> +       {
> +         name = l->function_name;
> +         continue;
> +       }
> +      if (strcmp (name, l->function_name) == 0)
> +       return 1;
> +    }

Assume we have location with function names like "a" "b", "b".
IIUC, this code will compare "a" with "b", and then "a" with "b"
again, and return 0. It will never compare "b" with "b". Am I wrong?
If I'm right we probably should have double loop.

> +
> +  return 0;
> +}
> +
> static void
> update_breakpoint_locations (struct breakpoint *b,
> struct symtabs_and_lines sals)
> @@ -7558,18 +7589,37 @@ update_breakpoint_locations (struct brea
> /* If possible, carry over 'disable' status from existing breakpoints.  */
> {
> struct bp_location *e = existing_locations;
> +    /* If there are multiple breakpoints with the same function name,
> +       e.g. for inline functions, comparing function names won't work.
> +       Instead compare pc addresses; this is just a heuristic as things
> +       may have moved, but in practice it gives the correct answer
> +       often enough until a better solution is found.  */
> +    int have_ambiguous_names = ambiguous_names_p (b->loc);
> +
> for (; e; e = e->next)
> {
> if (!e->enabled && e->function_name)
.......
> +           if (have_ambiguous_names)
> +             {

Do we still have to check for e->function name if have_ambiguous_names
is true?


> Index: testsuite/gdb.cp/mb-inline.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.cp/mb-inline.h,v
> diff -u -p -N mb-inline.h
> --- .//dev/null 1969-12-31 16:00:00.000000000 -0800
> +++ ./testsuite/gdb.cp/mb-inline.h      2007-10-15 16:40:42.908114000
> -0700 @@ -0,0 +1,10 @@
> +// Test gdb support for setting file:line breakpoints on inline fns.
> +
> +static inline int
> +foo (int i)

As I've asked on IRC -- is it important that this function is "static"?
Will the test be valid if "static" is removed. If so, can you add a 
comment explaining why?

Thanks,
Volodya