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

["Blanc, Nicolas" <nicolas.blanc@intel.com>]

Hi Pedro,

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

I choose to restrict the scope of the command to user-added files in order to
limit potential unforeseen side-effects in a first time. This restriction
can be lifted in the future if needed.

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

This is a good question. I added this function because update_solib_list handles the case of user-added files:

/* Unless the user loaded it explicitly, free SO's objfile.  */
          if (gdb->objfile && ! (gdb->objfile->flags & OBJF_USERLOADED)
              && !solib_used (gdb))
            free_objfile (gdb->objfile);

If the user-loaded check above is needed then remove_user_added_objfile() is also needed.
But I don't think it is currently possible that user-loaded files end up in so_list_head either.

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

Ok

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

Ok, I am using text_addr now.

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

I understand your point. GDB's implementation\doc assume that addr_low is the text address.
It' not only the case for add-symbol-file but also for info shared.

So I've added the following comment to struct objfile for clarification:

/* The base address of the object file, which by default is assumed to be
   the address of the text section.  The remove-symbol-file command uses
   this field to identify the object file to remove.  */

    CORE_ADDR addr_low;

I hope this helps.

Thanks,

Nicolas

--
Pedro Alves

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