Re: [PATCH] gdb: New maint info line-table command.

[Pedro Alves <palves at redhat dot com>]

Given this is a maint command, I'm fine with it as is.

I have a few suggestions below though.

On 02/06/2016 10:16 PM, Andrew Burgess wrote:

> +/* Implement the 'maint info line-table' command.  */
> +
> +static void
> +maintenance_info_line_tables (char *arg, int from_tty)
> +{
> +  struct ui_out *uiout = current_uiout;
> +  struct symtab *symtab = NULL;
> +  struct linetable *linetable;
> +  struct cleanup *table_cleanup;
> +  int i;
> +
> +  if (arg != NULL)
> +    symtab = lookup_symtab (arg);

Given there may be multiple symtabs with the same file name, I'd think it
better if this printed information on all symtabs that matched.

Basically, move the line table printing to a helper function, and then
use iterate_over_symtabs instead of lookup_symtab.

> +  else
> +    {
> +      struct frame_info *frame;
> +      CORE_ADDR pc;
> +
> +      frame = get_selected_frame (_("No frame selected."));
> +      pc = get_frame_address_in_block (frame);
> +      symtab = find_pc_line_symtab (pc);
> +    }
> +
> +  if (symtab == NULL)
> +    {
> +      if (arg)
> +	error (_("No matching symbol table found for `%s'."), arg);
> +      else
> +	error (_("No matching symbol table found."));
> +    }
> +
> +  linetable = SYMTAB_LINETABLE (symtab);
> +  if (linetable == NULL)
> +    error (_("Symbol table for `%s' has no line table."),
> +	   symtab_to_filename_for_display (symtab));
> +  if (linetable->nitems <= 0)
> +    error (_("Line table for symbol table `%s' has no lines"),
> +	   symtab_to_filename_for_display (symtab));

Seems odd for these to be errors.  Consider a gdb script or user-command
that does:

 maint info line-table
 maint info symtabs

If there's no line table, should the whole script error out, or just
print that as info?  I think the latter.

> +
> +  ui_out_text (uiout, "Line table for `");
> +  ui_out_text (uiout, symtab_to_filename_for_display (symtab));
> +  ui_out_text (uiout, "':\n");

(It'd probably be useful to also show the full name, to be sure you're
looking at the symtab you think you're looking at.)

Thanks,
Pedro Alves