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

[Justin Lebar <justin dot lebar at gmail dot com>]

On Fri, Jun 25, 2010 at 2:56 PM, Tom Tromey <tromey@redhat.com> wrote:
> FWIW I think this is a very good first patch to gdb.

Thanks!

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

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

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

I'm happy to switch to a hashtable here if that's what we want.

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

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

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

> Justin> This patch also fixes bug 11614: decode_variable() in linespec.c
> Justin> does not obey its contract
>
> It is somewhat preferred in gdb to submit things like this as separate
> patches.

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

> Justin> +      if (sals.nelts == 0)
> Justin> +       error (_("No function to blacklist.")); /* TODO can I trigger this? */
> Justin> +      if (sals.nelts > 1)
> Justin> +       error (_("Specify just one function at a time.")); /* TODO can I
> Justin> trigger this? */
>
> Good questions :-)
>
> It is great to be defensive here, but the comments should go before
> checkin.
>
> Maybe you can get nelts>1 with a C++ destructor?  I'm not sure.

I didn't hit that error with a simple C++ destructor.  Maybe some kind of
pathological destructor could hit it.

I'm mostly concerned that the error messages are meaningful if we do hit them.
I'm only guessing at what nelts == 0 and nelts > 1 mean.

> Justin> +      if (opts.addressprint)
> Justin> +       {
> 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> +       }
>
> A small nit, but I'm not sure about the exact string "n/a" here.
> If there is an analogous situation elsewhere it would be good to just do
> whatever is done there.

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

> 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. */
>
> Let's figure this out.

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

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

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

Hm.  How do we handle this with breakpoints?

> Justin> +  add_cmd ("enable", class_blacklist, blacklist_enable_command, _("\
> Justin> +Enable a blacklist entry.\n\
> Justin> +blacklist enable [NUMBER]"),
> Justin> +          &blacklistlist);
>
> I think it would be more usual to make the commands "enable blacklist ..."
> instead of "blacklist enable ...".  Likewise for disable and delete.

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

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

> Justin> -int default_breakpoint_valid;
> Justin> -CORE_ADDR default_breakpoint_address;
> Justin> -struct symtab *default_breakpoint_symtab;
> Justin> -int default_breakpoint_line;
> Justin> -struct program_space *default_breakpoint_pspace;
>
> I'm not sure I understand the rationale for this set of changes.
> Is it just to make the naming more clear?

Right: They're no longer the default breakpoint values since they're now also
the default blacklist values too.  I moved them into stack.c so that
set_last_displayed_codepoint (which replaces set_default_breakpoint) could be a
static function -- it's only called in stack.c.

-Justin

Attachment: bug8287
Description: Binary data

Attachment: bug11614
Description: Binary data