Re: [PATCH v3 6/8] gdb/remote: Parse tdesc field in stop reply and threads list XML

[Simon Marchi <simark@simark.ca>]

On 2/3/23 06:27, Luis Machado wrote:
> On 2/1/23 20:16, Simon Marchi via Gdb-patches wrote:
>>> IIUC, the tdescs would be deleted during the
>>> pop_all_targets_at_and_above, when the refcount of the remote_target
>>> gets to 0 and it gets deleted.  And the threads would be removed in
>>> generic_mourn_inferior just after.
>>>
>>> An idea could be to call generic_mourn_inferior before
>>> remote_unpush_target (no idea if it works).  Another one would be to
>>> get a temporary reference to the remote_target object in
>>> remote_unpush_target, just so that it outlives the threads.
>>> Or maybe we should say that it's a process target's responsibility to
>>> delete any thread it "owns" before getting deleted itself.
>>
>> Another question related to this popped while reading the following
>> patch.  When creating a gdbarch from a tdesc, the gdbarch keeps a
>> pointer to that tdesc (accessible through gdbarch_target_desc).  And
>> AFAIK, we never delete gdbarches.  So I suppose the gdbarch will refer a
>> stale target desc.  At first I thought it wouldn't be a problem in
>> practice, because while that gdbarch object still exists, nothing
>> references it (it is effectively leaked).  But then I remember that we
>> cache gdbarches to avoid creating arches with duplicate features.  So
>> later (let's say if you connect again to a remote), we might want to
>> create a gdbarch with the same features as before, and we'll dig up the
>> old gdbarch, that points to the now deleted tdesc.
> 
> The target descriptions for aarch64 are all cached using a map in gdb/aarch64-tdep.c:
> 
> /* All possible aarch64 target descriptors.  */
> static std::unordered_map <aarch64_features, target_desc *> tdesc_aarch64_map;
> 
> I don't think we should try to delete those, and they should live throughout the life of gdb (unless things get large, then we might consider cleanups).

When debugging natively with GDB, that's true.  When debugging remotely,
on GDBserver-side, that's true too.  But when debugging remotely, on
GDB-side, don't we create a new target_desc object for each read target
description?

Ok, I just saw in xml-tdesc.c:

  /* A record of every XML description we have parsed.  We never discard
     old descriptions, because we never discard gdbarches.  As long as we
     have a gdbarch referencing this description, we want to have a copy
     of it here, so that if we parse the same XML document again we can
     return the same "struct target_desc *"; if they are not singletons,
     then we will create unnecessary duplicate gdbarches.  See
     gdbarch_list_lookup_by_info.  */

  static std::unordered_map<std::string, target_desc_up> xml_cache;

So, at least, a remote sending the same exact XML over and over will
lead to the same target_desc object being reused.  And there won't be
lifetime issues, since the target_desc created from XML also live
forever.  So I guess we're good.

Simon