Re: [patch v4 1/3] Create remove-symbol-file command.

[Luis Machado <lgustavo@codesourcery.com>]

Hi,

A few nits and a suggestion at the end.

On 05/29/2013 11:13 AM, Nicolas Blanc wrote:
> Create remove-symbol-file command for removing
> symbol files added via the add-symbol-file command.
>
> 2013-18-03  Nicolas Blanc  <nicolas.blanc@intel.com>
>
> 	* breakpoint.c (is_addr_in_objfile): Create static helper function.

Shouldn't this say "New static inline function." instead?

> 	(disable_breakpoints_in_freed_objfile): Create function for disabling
> 	breakpoints in objfiles upon free_objfile notifications.

Likewise?

> 	* solib.c (remove_user_added_objfile): Create function for removing
> 	dangling references upon notification of free_objfile.

Likewise?

> 	* symfile.c (add_symbol_file_command): Set OBJFILE->LOW_ADDR.
> 	(remove_symbol_file_command): Create command for removing symbol files.

Likewise?

> +/* Return 1 if ADDR maps in one of the sections of OBJFILE and 0
> +   otherwise.  OBJFILE must be valid.  */

"maps into one of the sections" maybe?

> +
> +static inline int
> +is_addr_in_objfile (CORE_ADDR addr, const struct objfile * objfile)
> +{
> +  struct obj_section *osect;

If OBJFILE must be valid, how about calling gdb_assert() here to ensure 
it is indeed valid?

Unless not having a valid OBJFILE is a possibility.

> diff --git a/gdb/objfiles.h b/gdb/objfiles.h
> index 93149e2..644e780 100644
> --- a/gdb/objfiles.h
> +++ b/gdb/objfiles.h
> @@ -204,6 +204,10 @@ struct objfile
>
>       char *name;
>
> +    /* The base address of the object file, which is often assumed to be
> +       the address of the text section.  The remove-symbol-file command
> +       uses this field to identify the object file to remove.  */
> +
>       CORE_ADDR addr_low;
>
>       /* Some flag bits for this objfile.

No space between the variable and its comment block.

> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index e9609b2..7c5989c 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -2175,6 +2175,8 @@ add_symbol_file_command (char *args, int from_tty)
>     int expecting_sec_name = 0;
>     int expecting_sec_addr = 0;
>     char **argv;
> +  struct objfile *objf;
> +  CORE_ADDR addr_low;
>
>     struct sect_opt
>     {
> @@ -2307,12 +2309,17 @@ add_symbol_file_command (char *args, int from_tty)
>       }
>     section_addrs->num_sections = sec_num;
>
> +  addr_low = section_addrs->other[0].addr;
> +

Looks a little cryptic without a comment stating what this is/will be 
used for.

> +
> +/* This function removes a symbol file that was added via add-symbol-file.  */
> +
> +static void
> +remove_symbol_file_command (char *args, int from_tty)
> +{
> +  CORE_ADDR addr = 0;
> +  struct objfile* objf;
> +  struct gdbarch *gdbarch = get_current_arch ();
> +
> +  dont_repeat ();
> +
> +  if (args == NULL)
> +    error (_("USAGE: remove-symbol-file <text_address>"));
> +
> +  addr = parse_and_eval_address (args);
> +
> +  ALL_OBJFILES (objf)
> +  {
> +    if (objf->flags & OBJF_USERLOADED && objf->addr_low == addr)
> +      break;
> +  }
> +
> +  if (objf == NULL)
> +    error (_("no user-added symbol file for .text_addr = 0x%s"),
> +	   phex_nz (addr, sizeof (addr)));
> +
> +  printf_unfiltered (_("Remove symbol table from file \"%s\"\
> + at .text_addr = %s\n"),
> +		     objf->name, paddress (gdbarch, addr));
> +
> +  if (from_tty && (!query ("%s", "")))
> +    error (_("Not confirmed."));
> +
> +  free_objfile (objf);
> +  clear_symtab_users (0);
> +
> +}

Empty line before the end of the function should be gone.


> @@ -3734,6 +3781,12 @@ with the text.  SECT is a section name to be loaded at SECT_ADDR."),
>   	       &cmdlist);
>     set_cmd_completer (c, filename_completer);
>
> +  c = add_cmd ("remove-symbol-file", class_files,
> +	       remove_symbol_file_command, _("\
> +Remove a symbol file loaded via the add-symbol-file command.\n\
> +Usage: remove-symbol-file ADDR.\nADDR is the starting address of the\
> + text section of the file to remove."), &cmdlist);
> +
>     c = add_cmd ("load", class_files, load_command, _("\
>   Dynamically load FILE into the running program, and record its symbols\n\
>   for access from GDB.\n\
>

I'm thinking, with a command called "remove-symbol-file" i would expect 
to provide some kind of filename to this command. Should the user also 
be able to state a DSO name here and have it unloaded? Maybe have the 
name translated to the base address used to unload the library internally?

Luis