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

[Pedro Alves <palves at redhat dot com>]

On 04/29/2013 06:30 PM, Blanc, Nicolas wrote:
> 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.

That's not what I meant.  I agree with restrict the scope of the
command to user-added files, and I'd probably voice against it if
the patch did otherwise.  The question is why does this piece of code
need to be aware of this?  As in, the "free_objfile" notification is called
irrespective of the objfile being a user loaded objfile or not.
So a reader seeing this has to wonder "okay, then if a !userloaded
objfile is freed, what is making sure we don't end up with stale
references to that freed objfile, if not this function?"  We need at
least a comment clarifying this.

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

git blame points at this:
  http://sourceware.org/ml/gdb-patches/2000-03/msg00273.html

Which seems to be the initial implementation for dlclose support,
very early shared library support code.  There's lots of discussion
around that patch, including several different implementations of the support:

In both
http://sourceware.org/ml/gdb-patches/2000-03/
and:
http://sourceware.org/ml/gdb/2000-03/

(look for dlclose and unloading shared objects).

An earlier version of the patch didn't have that bit, but I didn't
find the rationale for adding it in the final version.

The reason I can think of, is that the user may do:

(gdb) add-symbol-file /foo/bar.so ADDR

and if the program itself later loads /foo/bar.so,
then the code that loads the objfile for the DSO finds
that an objfile for bar.so has already been loaded, and
reuses it.  At the time of that initial patch in 2000,
the code that checked whether the objfile was already
loaded just compared names:

  /* Have we already loaded this shared object?  */
  ALL_OBJFILES (so->objfile)
    {
      if (strcmp (so->objfile->name, so->so_name) == 0)
	return 1;
    }

So that seems likely to be the rationale to me.

The current code hasn't changed that much.  It's in
solib_read_symbols:

	  /* Have we already loaded this shared object?  */
	  ALL_OBJFILES (so->objfile)
	    {
	      if (filename_cmp (so->objfile->name, so->so_name) == 0
		  && so->objfile->addr_low == so->addr_low)
		break;
	    }
	  if (so->objfile != NULL)
	    break;

...
	  so->objfile = symbol_file_add_from_bfd (so->abfd,
						  flags, sap, OBJF_SHARED,
						  NULL);
	  so->objfile->addr_low = so->addr_low;


We just gained the addr_low extra check.  However, it still seems
possible the solib code reuses a user added objfile...

I think we should define precisely what does it mean when
the user does add-symbol-file and then the program loads the
same file.

Sorry, I haven't thought on this as much as I wanted, but I
have to go now.  I'll possibly respond more tomorrow.

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


-- 
Pedro Alves