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

["Blanc, Nicolas" <nicolas dot blanc at intel dot com>]

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

Tom> Two periods after space -- Yao pointed out one instance of this, but there are a couple.
Nicolas> I'll fixed those.

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

Tom> I liked Yao's comment about notifying just once per breakpoint.  It doesn't seem difficult to do that.
Nicolas> Ok I'll use a list of breakpoints to collect first the breakpoints to notify.

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

Nicolas> First disable_breakpoints_in_free_objfile() is called and then breakpoint_free_objfile() from free_objfile().
Nicolas> Now I could test moving the code from breakpoint_free_objfile() to disable_breakpionts_in_free_objfile() and delete breakpoint_free_objfile().


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

> Tom> It isn't totally clear to me that these are equivalent.
> Tom> From browsing solib.c it seems that an solib can be freed without the objfiles being purged.
> Tom> 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> Indeed, an object file is shared with another SO or it was loaded by the user then free_so() will be called but not free_objfile().
Nicolas> In that case clear_dangling_disaplay_expression() won't be called. I think it's ok because the dangling reference that needs
Nicolas> to be cleaned-up is the object file not the SO.

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

Tom >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> Ok, I'll see what happens when I remove it.

> Nicolas> +	    addr_low = parse_and_eval_address (arg);

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

Nicolas> You're right. I can take the value later on from the subsequent loop.

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

Tom> Is there any way to display this information?

Nicolas> Unfortunately the answer no. "info files" does not list user-added files. GDB is currently lacking commands to show user-added files.

Tom> Otherwise, how should users find it?
Nicolas> The user knows the address because he typed it in for adding the file. Note that when the user needs adding a file manually then he has a way outside GDB to learn about the memory mapping.

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

Tom> 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.
Tom>I realize this is how add-symbol-file works, but I think it is a bad approach.

Nicolas> Ok, I'll do that.

> Nicolas> +  if (!objf)
Tom> We recently decided to use the wordier "objf != NULL" style.

Nicolas> Ok.

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

Tom> I think this line is too long.

Nicolas> Will be fixed.

Thanks for your feedback. It's much appreciated.

Nicolas
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052