Re: [patch v6 1/3] New remove-symbol-file command.

[Luis Machado <lgustavo@codesourcery.com>]

Thanks.

It is looking good. A few more comments below.

On 06/05/2013 09:07 PM, Nicolas Blanc wrote:
> New command for removing symbol files added via
> the add-symbol-file command.
>
> 2013-18-03  Nicolas Blanc  <nicolas.blanc@intel.com>
>
> 	* breakpoint.c (disable_breakpoints_in_freed_objfile): New function for
> 	disabling breakpoints in objfiles upon free_objfile notifications.
> 	* objfiles.c (free_objfile): Notify free_objfile.
> 	(is_addr_in_objfile): New query function.
> 	* objfiles.h (is_addr_in_objfile): New declaration.
> 	* printcmd.c (clear_dangling_display_expressions): Act upon free_objfile
> 	events instead of solib_unloaded events.
> 	(_initialize_printcmd): Register observer for free_objfile instead
> 	of solib_unloaded notifications.
> 	* solib.c (remove_user_added_objfile): New function for removing
> 	dangling references upon notification of free_objfile.
> 	* symfile.c (remove_symbol_file_command): New command for removing symbol
> 	files.
> 	(_initialize_symfile): Add remove-symbol-file.
> gdb/doc
> 	* observer.texi: New free_objfile event.

For new functions, "New function" should do. No need to explain the 
functions in the ChangeLog entry.

>
> Signed-off-by: Nicolas Blanc <nicolas.blanc@intel.com>
> ---
>   gdb/breakpoint.c      |   66 ++++++++++++++++++++++++++++++++++++--
>   gdb/doc/observer.texi |    4 ++
>   gdb/objfiles.c        |   23 +++++++++++++
>   gdb/objfiles.h        |    2 +
>   gdb/printcmd.c        |   15 +++++---
>   gdb/solib.c           |   22 +++++++++++++
>   gdb/symfile.c         |   84 +++++++++++++++++++++++++++++++++++++++++++++++++
>   7 files changed, 207 insertions(+), 9 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index d4ccc81..9446918 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -7403,9 +7403,9 @@ disable_breakpoints_in_shlibs (void)
>     }
>   }
>
> -/* Disable any breakpoints and tracepoints that are in an unloaded shared
> -   library.  Only apply to enabled breakpoints, disabled ones can just stay
> -   disabled.  */
> +/* Disable any breakpoints and tracepoints that are in SOLIB upon
> +   notification of unloaded_shlib.  Only apply to enabled breakpoints,
> +   disabled ones can just stay disabled.  */
>
>   static void
>   disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
> @@ -7457,6 +7457,65 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
>     }
>   }
>
> +/* Disable any breakpoints and tracepoints in OBJFILE upon
> +   notification of free_objfile.  Only apply to enabled breakpoints,
> +   disabled ones can just stay disabled.  */
> +
> +static void
> +disable_breakpoints_in_freed_objfile (struct objfile * objfile)
> +{
> +  struct breakpoint *b;
> +
> +  if (objfile == NULL)
> +    return;
> +
> +  /* If the file is a shared library not loaded by the user then
> +     solib_unloaded was notified and disable_breakpoints_in_unloaded_shlib
> +     was called.  In that case there is no need to take action again.  */
> +  if ((objfile->flags & OBJF_SHARED) && !(objfile->flags & OBJF_USERLOADED))
> +    return;
> +
> +  ALL_BREAKPOINTS (b)
> +  {
> +    struct bp_location *loc;
> +    int bp_modified = 0;
> +    int is_no_tracepoint = !is_tracepoint (b);
> +
> +    if (is_no_tracepoint
> +	&& b->type != bp_breakpoint
> +	&& b->type != bp_jit_event
> +	&& b->type != bp_hardware_breakpoint)
> +      continue;

Sorry i didn't notice this, but, does it make sense to use the 
is_breakpoint function here? It checks for bp_breakpoint, 
bp_hardware_breakpoint and bp_dprintf.

> +
> +    for (loc = b->loc; loc != NULL; loc = loc->next)
> +      {
> +	CORE_ADDR loc_addr = loc->address;
> +
> +	if (is_no_tracepoint
> +	    && loc->loc_type != bp_loc_hardware_breakpoint
> +	    && loc->loc_type != bp_loc_software_breakpoint)
> +	  continue;
> +
> +	if (loc->shlib_disabled != 0)
> +	  continue;
> +
> +	if (objfile->pspace != loc->pspace)
> +	  continue;
> +
> +	if (is_addr_in_objfile (loc_addr, objfile))
> +	  {
> +	    loc->shlib_disabled = 1;
> +	    loc->inserted = 0;
> +	    bp_modified = 1;
> +	  }
> +      }
> +
> +    if (bp_modified)
> +      observer_notify_breakpoint_modified (b);
> +  }
> +
> +}
> +

Since we are disabling breakpoints that are related to a specific 
objfile or shared library, i think we should call either 
mark_breakpoint_modified or mark_breakpoint_location_modified to make 
sure things still work OK in case any of these breakpoints are carrying 
conditions and the evaluation of these conditions is taking place on the 
target's side.

In fact, i think we may be missing calls to these functions in a couple 
other places, but you don't need to address those.

> diff --git a/gdb/solib.c b/gdb/solib.c
> index c987fe5..987d510 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -1478,6 +1478,26 @@ gdb_bfd_lookup_symbol (bfd *abfd,
>     return symaddr;
>   }
>
> +/* SO_LIST_HEAD may contain user-loaded object files that can be removed
> +   out-of-band by the user.  So upon notification of free_objfile remove
> +   any reference to any user-loaded file that is about to be freed.  */
> +
> +static void
> +remove_user_added_objfile (struct objfile *objfile)
> +{
> +  struct so_list *so;
> +
> +  if (!objfile)
> +    return;
> +

Just a nit. Keep everything consistent in the code. Above you used 
"objfile == NULL" instead of "!objfile".