Re: [PATCH 1/3] Added command remove-symbol-file.

[Yao Qi <yao@codesourcery.com>]

On 04/16/2013 03:51 PM, Nicolas Blanc wrote:
> Added command remove-symbol-file for removing
> symbol files added via the add-symbol-file command.
>

Nicolas,
The patch looks good to me, but I am not the people to approve it.  Some 
nits below,

> 2013-18-03  Nicolas Blanc  <nicolas.blanc@intel.com>
>
> 	* breakpoint.c (disable_breakpoints_in_free_objfile): Created
> 	function for disabling breakoints in objfiles upon FREE_OBJFILE
> 	notifications.
> 	* doc/observer.text: Created FREE_OBJFILE event.

Change to doc/observer.texi should be moved to a separate changelog entry.

> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index f374368..d5c7b21 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
>
> +/* Disable any breakpoints and tracepoints in OBJFILE upon
> +   notification of FREE_OBJFILE. Only apply to enabled breakpoints,
                                    ^^ Miss one space after period.

> +   disabled ones can just stay disabled.  */
> +
> +static void
> +disable_breakpoints_in_free_objfile (struct objfile * objfile)
> +{
> +  struct bp_location *loc, **locp_tmp;
> +
> +  if (objfile == NULL)
> +    return;
> +
> +  /* If the file is a shared library not loaded by the user then
> +     SOLIB_UNLOADED was notified and DISABLE_BREAKPIONTS_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_BP_LOCATIONS (loc, locp_tmp)
> +  {
> +    struct obj_section *osect;
> +    CORE_ADDR loc_addr = loc->address;
> +
> +    /* ALL_BP_LOCATIONS bp_location has LOC->OWNER always non-NULL.  */
> +    struct breakpoint *b = loc->owner;
> +
> +    if (loc->shlib_disabled != 0)
> +      continue;
> +
> +    if (objfile->pspace != loc->pspace)
> +      continue;
> +
> +    if (!is_tracepoint(b))
> +      {
> +	if (b->type != bp_breakpoint
> +	    && b->type != bp_jit_event
> +	    && b->type != bp_hardware_breakpoint)
> +	  continue;
> +
> +	if (loc->loc_type != bp_loc_hardware_breakpoint
> +	    && loc->loc_type != bp_loc_software_breakpoint)
> +	  continue;
> +      }
> +
> +    ALL_OBJFILE_OSECTIONS (objfile, osect)
> +    {
> +      if (obj_section_addr (osect) <= loc_addr
> +	  && loc_addr < obj_section_endaddr (osect))
> +	{
> +	  loc->shlib_disabled = 1;
> +	  loc->inserted = 0;
> +	  observer_notify_breakpoint_modified (loc->owner);

Nit: this observer will be called multiple times for a single breakpoint 
LOC->owner, if it has multiple breakpoint locations in this objfile.  It 
is ideal to call observer only once for a breakpoint.  Your patch is OK 
to me as well.

> diff --git a/gdb/doc/observer.texi b/gdb/doc/observer.texi
> index adb7085..7b420ea 100644
> --- a/gdb/doc/observer.texi
> +++ b/gdb/doc/observer.texi
> @@ -138,6 +138,10 @@ Called with @var{objfile} equal to @code{NULL} to indicate
>   previously loaded symbol table data has now been invalidated.
>   @end deftypefun
>
> +@deftypefun void free_objfile (struct objfile *@var{objfile})
> +The symbol file specified by @var{objfile} is about to be freed.

I feel that "symbol file" is not accurate.  Maybe "object file"?

> +@end deftypefun
> +
>   @deftypefun void new_thread (struct thread_info *@var{t})
>   The thread specified by @var{t} has been created.
>   @end deftypefun

> index 2cc33d0..e6f62ba 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -1928,21 +1928,22 @@ disable_display_command (char *args, int from_tty)
>      an item by re-parsing .exp_string field in the new execution context.  */
>
>   static void
> -clear_dangling_display_expressions (struct so_list *solib)
> +clear_dangling_display_expressions (struct objfile *objfile)
>   {
> -  struct objfile *objfile = solib->objfile;
>     struct display *d;
> +  struct program_space *pspace;
>
>     /* With no symbol file we cannot have a block or expression from it.  */
>     if (objfile == NULL)
>       return;
> +  pspace = objfile->pspace;
>     if (objfile->separate_debug_objfile_backlink)
>       objfile = objfile->separate_debug_objfile_backlink;
> -  gdb_assert (objfile->pspace == solib->pspace);
> +  gdb_assert (objfile->pspace == pspace);
>

This assert is always true.  Probably, we need to move this assert to 
the callers of "free_objfile" when argument is solib->objfile.

>     for (d = display_chain; d != NULL; d = d->next)
>       {
> -      if (d->pspace != solib->pspace)
> +      if (d->pspace != pspace)
>   	continue;
>
>         if (lookup_objfile_from_block (d->block) == objfile
> @@ -2474,7 +2475,7 @@ _initialize_printcmd (void)
>
>     current_display_number = -1;
>
> -  observer_attach_solib_unloaded (clear_dangling_display_expressions);
> +  observer_attach_free_objfile (clear_dangling_display_expressions);
>
>     add_info ("address", address_info,
>   	    _("Describe where symbol SYM is stored."));
> diff --git a/gdb/solib.c b/gdb/solib.c
> index 6978677..b0c2f04 100644
> --- a/gdb/solib.c
> +++ b/gdb/solib.c
> @@ -1442,6 +1442,30 @@ gdb_bfd_lookup_symbol (bfd *abfd,
>     return symaddr;
>   }
>
> +/* Upon notification of FREE_OBJFILE remove any reference
> +   to any user-added file that is about to be freed.  */

A blank line is needed here.

> +static void
> +remove_user_added_objfile (struct objfile *objfile)
> +{
> +  struct so_list *gdb;
> +
> +  if (!objfile)
> +    return;
> +
> +  if (!(objfile->flags & OBJF_USERLOADED)
> +      || !(objfile->flags & OBJF_SHARED))
> +    return;
> +
> +  gdb = so_list_head;
> +  while (gdb)
> +    {
> +      if (gdb->objfile == objfile)
> +	gdb->objfile = NULL;
> +      gdb = gdb->next;
> +    }
> +}
> +

> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 3e66bd1..273c07a 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -2364,6 +2372,56 @@ add_symbol_file_command (char *args, int from_tty)
>   }
>   \f
>
> +
> +/* This function removes a symbol file that was added via add-symbol-file.  */

A blank line is needed here.

> +static void
> +remove_symbol_file_command (char *args, int from_tty)
> +{

-- 
Yao (齐尧)