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

["Douglas Evans" <dje at google dot com>]

On Nov 13, 2007 1:10 PM, Vladimir Prus <ghost@cs.msu.su> wrote:
> > 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.

Correct.  I can fix the wording.

> > +/* 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.  */

Well ...
"Non-zero return means we cannot use ..." is an attribute of the
caller.  We can certainly put such a comment at the call site.
There's a comment to that effect at the call site already though.

How about simply

+/* Return non-zero if multiple fns in list LOC have the same name.
+   Null names are ignored.  */

> 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.

Like the original comment to the function says, it wasn't written to
handle multiple different names.  An alternative to two loops is to
store all the names in a set and then see if the set has more than one
member.

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

I wrote it that way to preserve the basic intent of the loop.
I can rewrite it as one pleases however.

> 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?

"static inline" is a pretty common gcc idiom.
bash$ find /usr/include -type f  | xargs grep "static inline"

Is this what you're looking for?

+/* This is "static inline" to cause the function to be put
+   in each file that it is compiled in.  */
+static inline int
+foo (int i)
+{
+  return i; // set breakpoint here
+}

Maybe I should remove the "inline" so the test is immune to -O.