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

[Pedro Alves <palves@redhat.com>]

On 04/16/2013 08:51 AM, Nicolas Blanc wrote:
> 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.

Present tense "create", etc.  (throughout the log entry).

Typo breakoints.

> 	* doc/observer.text: Created FREE_OBJFILE event.
> 	* objfiles.c (free_objfile): Notify FREE_OBJFILE.
> 	* 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): Created function for removing
> 	dangling references upon notification of FREE_OBJFILE.
> 	* symfile.c (add_symbol_file_command): Set OBJFILE->LOW_ADDRESS.

LOW_ADDR.

> 	(remove_symbol_file_command): Created command for removing symbol files.
> 	(_initialize_symfile): Added remove-symbol-file.


> +/* Upon notification of FREE_OBJFILE remove any reference
> +   to any user-added file that is about to be freed.  */

Why only user-added files?

> +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;
> +    }

Or rather/also, this looks a bit weird to me.
Can we ever really ever find a user-loaded file in the
so_list_head list?  What would that mean?
IIRC, the only way to get a OBJF_USERLOADED|OBJF_SHARED
objfile is through "dll-symbols" (dll_symbol_command),
but that doesn't create any entry in the shared library list.

BTW, calling the variable "gdb" was probably copied from
solib.c:update_solib_list.  It makes sense to call it
that to contrast with "inferior", but not here.  Just call
it something like "so".  That while loop can be written
simpler as a for loop:

  for (so = so_list_head; so != NULL; so = so->next)
     if (so->objfile == objfile)
	so->objfile = NULL;


> +static void
> +disable_breakpoints_in_free_objfile (struct objfile * objfile)

This is clearly mirroring the naming of
disable_breakpoints_in_unloaded_shlib.  Should be "in freed objfile".
"in free objfile" would mean something else.


> +  /* If the file is a shared library not loaded by the user then
> +     SOLIB_UNLOADED was notified and DISABLE_BREAKPIONTS_IN_UNLOADED_SHLIB

Typo BREAKPIONTS.  Actually, uppercase is used when referring to
a value of a variable (see GNU coding standards), which is not the
case here.

> +	error (_("USAGE: remove-symbol-file <text_low_address>"));

I'd s/low// here.  text_address is clear and common enough that
having "low" there makes me go "what does low mean here?"  Or just
<address> even.

> +    if (objf->flags & OBJF_USERLOADED && objf->addr_low == addr)

As I mentioned, the .text address may not be the lower address
in the object at all, so this "addr_low" confused me.
I'd be happier with naming the field for what ig really is, something
like "add_symbol_file_addr", with a comment indicating this is related
to "add-symbol-file", but I see you're reusing an existing variable.  At
the very least, the variable's definition should gain a comment explaining
its overloading for add-symbol-file.

-- 
Pedro Alves