From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1750 invoked by alias); 13 Nov 2007 21:10:27 -0000 Received: (qmail 1737 invoked by uid 22791); 13 Nov 2007 21:10:26 -0000 X-Spam-Check-By: sourceware.org Received: from main.gmane.org (HELO ciao.gmane.org) (80.91.229.2) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 13 Nov 2007 21:10:24 +0000 Received: from list by ciao.gmane.org with local (Exim 4.43) id 1Is31X-00037U-8M for gdb-patches@sources.redhat.com; Tue, 13 Nov 2007 21:10:19 +0000 Received: from 77.246.241.246 ([77.246.241.246]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Tue, 13 Nov 2007 21:10:19 +0000 Received: from ghost by 77.246.241.246 with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Tue, 13 Nov 2007 21:10:19 +0000 To: gdb-patches@sources.redhat.com From: Vladimir Prus Subject: Re: RFA: patch to fix multi-breakpoint enable/disable handling of inline functions Date: Tue, 13 Nov 2007 21:10:00 -0000 Message-ID: References: <20071016001816.F059B1C7E69@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8Bit User-Agent: KNode/0.10.4 X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2007-11/txt/msg00253.txt.bz2 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   > > * 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