Re: [Patch] Bug 8287: Skip uninteresting functions while debugging

Tom Tromey
2010-07-20T21:03:54Z

>>>>> "Justin" == Justin Lebar <justin.lebar@gmail.com> writes:

Tom> One concern of mine is that this code will not perform well with many
Tom> blacklists in place. It may make sense to keep a hash table indexed by
Tom> name (function or file), to make lookups faster. What do you think?

Justin> I imagine you'd have to have a pretty large blacklist to notice
Justin> a delay when using gdb interactively, even on modest hardware.
Justin> I don't know much about gdb scripting -- this might matter more
Justin> in that case. But would one single-step in a script?

Justin> FWIW, enabling/disabling/deleting breakpoints takes time linear
Justin> to the number of breakpoints.

Yeah, good points, let's just leave it as is.

Tom> Another random thought is whether we want to be able to blacklist based
Tom> on objfile name.

Justin> That seems like it might be nice. Regular expression matching
Justin> on file and function names has also been suggested.

All good ideas; it is fine to defer these if you'd rather.

Justin> While I'm thinking about it, one problem I've run into while
Justin> using this patch while debugging Firefox is that we somehow load
Justin> multiple copies of our smart pointer header, so I have to
Justin> blacklist both ../../nsCOMPtr.h and ../../../nsCOMPtr.h. I need
Justin> to investigate further exactly what about our build system is
Justin> causing this to see if it's something which should be fixed in
Justin> gdb or Mozilla's build, but my guess is that we may want to
Justin> support this in gdb somehow.

Allowing regular expressions is a solid gdb tradition.

Justin> This patch also fixes bug 11614: decode_variable() in linespec.c
Justin> does not obey its contract

Tom> It is somewhat preferred in gdb to submit things like this as separate
Tom> patches.

Justin> I've split them out in this new set. Because you need the
Justin> bug11614 patch in order for the blacklist patch to work (and
Justin> since bug11614 is a trivial fix), I've included both attachments
Justin> in this one e-mail.

This patch is ok. Please check it in.

Mention the PR in the ChangeLog entry, like:

2010-06-25 Justin Lebar <justin.lebar@gmail.com>

PR gdb/11614:
* linespec.c (decode_variable): Passing a non-null not_found_ptr
[...]

If you do this, and then use the ChangeLog entry as the commit message,
the commit will show up in bugzilla.

Justin> + if (b->pc != 0)
Justin> + ui_out_field_core_addr (uiout, "addr", b->gdbarch, b->pc); /* 4 */
Justin> + else
Justin> + ui_out_field_string (uiout, "addr", "n/a"); /* 4 */
Justin> + }

Tom> A small nit, but I'm not sure about the exact string "n/a" here.
Tom> If there is an analogous situation elsewhere it would be good to just do
Tom> whatever is done there.

Justin> We could just use the empty string if that would be clearer.

Yeah, I think so.

Justin> + /* First, check whether the file or function this entry is pending on has
Justin> + been loaded. It might be more sensible to do this on a solib load,
Justin> + but that doesn't seem to work for some reason. */

Tom> Let's figure this out.

Justin> I spent a while in #gdb with a few people (I don't recall who)
Justin> and we couldn't figure this out. I added a solib load observer,
Justin> but from within it, find_pc_partial_function always failed.

Justin> I'm not sure where to look to figure out what's going on.

Offhand I'm not sure either, I think all you can do is debug the
particular call.

Tom> Also, since the blacklist includes a PC value, I think you have to reset
Tom> it when re-running the inferior, and on some other state changes as well.

Justin> Hm. How do we handle this with breakpoints?

See breakpoint_re_set.

Tom> I think it would be more usual to make the commands "enable blacklist ..."
Tom> instead of "blacklist enable ...". Likewise for disable and delete.

Justin> I'm not sure I like "enable blacklist". The problem is that
Justin> "blacklist" is a noun referring to the list of all functions we
Justin> skip and also a verb meaning "add an entry to the blacklist".
Justin> "Enable blacklist" sounds to me like we're enabling an entire
Justin> list of blacklist entries.

Yeah, that makes sense.

Justin> Maybe we can name the thing which indicates that we shouldn't
Justin> step into a function something less confusing than a "blacklist
Justin> entry"?

Yes, that would be good. Any ideas for names?

Tom