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

[Tom Tromey <tromey@redhat.com>]

>>>>> "Nicolas" == Nicolas Blanc <nicolas.blanc@intel.com> writes:

Nicolas> +/* Disable any breakpoints and tracepoints that are in SOLIB upon
Nicolas> +   notification of UNLOADED_SHLIB. Only apply to enabled breakpoints,

Two periods after space -- Yao pointed out one instance of this, but
there are a couple.

Nicolas> +    ALL_OBJFILE_OSECTIONS (objfile, osect)
Nicolas> +    {
Nicolas> +      if (obj_section_addr (osect) <= loc_addr
Nicolas> +	  && loc_addr < obj_section_endaddr (osect))
Nicolas> +	{
Nicolas> +	  loc->shlib_disabled = 1;
Nicolas> +	  loc->inserted = 0;
Nicolas> +	  observer_notify_breakpoint_modified (loc->owner);
Nicolas> +	  break;
Nicolas> +	}

I liked Yao's comment about notifying just once per breakpoint.  It
doesn't seem difficult to do that.

It's curious that this new function and breakpoint_free_objfile don't
overlap at all.

Nicolas> -  observer_attach_solib_unloaded (clear_dangling_display_expressions);
Nicolas> +  observer_attach_free_objfile (clear_dangling_display_expressions);

It isn't totally clear to me that these are equivalent.
From browsing solib.c it seems that an solib can be freed without the
objfiles being purged.

I am not sure but I think maybe the path core_close -> clear_solib can
do this.  If so, maybe this is simply a bug there; I am not sure.

Nicolas> +static void remove_symbol_file_command (char *, int);

I don't think you really need this declaration.  It's better to leave
them out unless they are needed for something specific, like mutual
recursion.  (There are lots of leftover ones, which I've always imagined
to be from a time when GCC's prototype warnings worked differently.)

Nicolas> +	    addr_low = parse_and_eval_address (arg);

It seems like this parses and evaluates this argument twice.
I think it would be better to take another approach, so that it is just
evaluated once.

Nicolas> +  /* Set the low address of the object for identification.  */
Nicolas> +  objf->addr_low = addr_low;

Is there any way to display this information?
Otherwise, how should users find it?

Nicolas> +static void
Nicolas> +remove_symbol_file_command (char *args, int from_tty)
[...]
Nicolas> +  argv = gdb_buildargv (args);
Nicolas> +  make_cleanup_freeargv (argv);

If the syntax is really just an expression as an argument, then don't
bother with gdb_buildargv.  Using gdb_buildargv means that complicated
expressions will have to be quoted unusually.

I realize this is how add-symbol-file works, but I think it is a bad
approach.

Nicolas> +  if (!objf)

We recently decided to use the wordier "objf != NULL" style.

Nicolas> +  c = add_cmd ("remove-symbol-file", class_files, remove_symbol_file_command, _("\

I think this line is too long.

Tom