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

[Tom Tromey <tromey@redhat.com>]

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

Justin> This adds support for a "blacklist" which contains files and
Justin> functions which are skipped while single-stepping.

Thanks for doing this.

FWIW I think this is a very good first patch to gdb.


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?

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

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.

Now for the nits ...

Justin> +#define ALL_BLACKLIST_ENTRIES(B) for (B = blacklist_entry_chain; B; B = B->next)

I think this should be wrapped before the 'for'.

Justin> +  b->filename = strdup(filename);

Space before open paren.  Also, you must use xstrdup, not strdup.  This
occurs more than once.

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.

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.

Justin> +int
Justin> +function_pc_is_blacklisted (CORE_ADDR pc)

All functions should have an introductory comment explaining its
contract -- purpose and meaning of arguments and result.  For functions
implementing commands or other helpers, this can be pretty short.

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.

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.

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.

Justin> --- /dev/null
Justin> +++ b/gdb/blacklist.h
[...]
Justin> +int
Justin> +function_pc_is_blacklisted (CORE_ADDR pc);

No newline after "int" in a declaration.

A header file should have a #if guard.

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?

I do like the new, more opaque, approach.

Justin> -  class_pseudo, class_tui, class_user, class_xdb
Justin> +  class_pseudo, class_tui, class_user, class_xdb, class_blacklist

I don't think you need a new command class for this.
I think class_breakpoint might be fine.

Tom