Re: [PATCH] gdb: Introduce user-friendly namespace identifier for "info shared"

[Guinevere Larsen <guinevere@redhat.com>]

On 3/17/25 12:36 PM, Simon Marchi wrote:
> I didn't do a full review, I can review more in depth a future version
> (since you'll probably change the behavior of the "info shared" column
> anyway).  I noted some high level things:
>
> On 3/13/25 1:00 PM, Guinevere Larsen wrote:
>> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
>> index 8378ecaff40..dabbd68d02e 100644
>> --- a/gdb/solib-svr4.c
>> +++ b/gdb/solib-svr4.c
>> @@ -405,11 +405,59 @@ struct svr4_info
>>        The special entry zero is reserved for a linear list to support
>>        gdbstubs that do not support namespaces.  */
>>     std::map<CORE_ADDR, std::vector<svr4_so>> solib_lists;
>> +
>> +  /* Mapping between r_debug[_ext] addresses and a user-friendly
>> +     identifier for the namespace.  We use both a vector and an unordered
>> +     map for a couple of reasons.  The vector is used to make it
>> +     easy to assign new internal IDs to namespaces, and to double check
>> +     which namespaces are still active.  The map is used when we're
>> +     rewriting the solib_lists, to make sure that IDs remain consistent
>> +     as SOs get dlclosed and deactivate namespaces.
>> +
>> +     For gdbservers that don't support namespaces, the first entry of the
>> +     vector will be nullptr, and the map will be empty.
>> +
>> +     A note on consistency. We can't make the IDs be consistent before
>> +     and after the initial relocation of the inferior (when the global
>> +     _r_debug is relocated, as mentioned in the previous comment).  It is
>> +     likely that this is a non-issue, since the inferior can't have called
>> +     dlmopen yet, but I think it is worth noting.
>> +
>> +     The only issue I am aware at this point is that, if when parsing an
>> +     XML file, we read an LMID that given by an XML file (and read in
>> +     library_list_start_library) is the identifier obtained with dlinfo
>> +     instead of the address of r_debug[_ext], and after attaching the
>> +     inferior adds another SO to that namespace, we might double-count it
>> +     since we won't have access to the LMID later on.  However, this is
>> +     already a problem with the existing solib_lists code.  */
>> +  std::vector<CORE_ADDR> namespace_id_vec;
>> +  std::unordered_map<CORE_ADDR, size_t> namespace_id_map;
> Since the count of namespaces is always expected to be small, you could
> consider just maintaining just a vector.  For example, glibc only
> supports a max of 16 namespaces today.  Maintaining one data structure
> is usually easier than maintaining two in parallel.

The issue here is handling dlclose calls. When svr4 detects a dlclose 
call, it will clear the solib_lists object, and read all the SOs again. 
So if the dlclose call has deactivated a namespace, we don't have an 
easy way to identify which namespaces were deactivated, and the way to 
handle this is to deactivate them all and reactivate them in 
svr4_maybe_add_namespace.

And we need all the data to reactivate namespaces to be stored in 
svr4_info, because if the target is a gdbserver, we're reading 
everything through svr4_current_sos_via_xfer_libraries, and it doesn't 
sound like a good idea to modify the xml parsing code to pass some 
context that would only be useful for solib-svr4.

While writing this, I realized I might be able to simplify this, having 
just a set of namespace IDs that are active, which will make the 
synchronization much simpler.

>
>> diff --git a/gdb/solist.h b/gdb/solist.h
>> index 9a157a4bbae..a6822877770 100644
>> --- a/gdb/solist.h
>> +++ b/gdb/solist.h
>> @@ -180,6 +180,17 @@ struct solib_ops
>>        name).  */
>>   
>>     std::optional<CORE_ADDR> (*find_solib_addr) (solib &so);
>> +
>> +  /* Return which linker namespace contains the current so.
> "the current so", or just "SO" (... which linker namespace contains SO)?
>
>> +     If the linker or libc does not support linkage namespaces at all
>> +     (which is basically all of them but solib-svr4), this function should
>> +     be set to nullptr, so that "info shared" won't add an unnecessary
>> +     column.
> I would not put a statement like "which is basically all of them but
> solib" in the interface here.  It is true until it isn't.  I think it's
> fine to sometimes mention a specific implementation in the interface
> documentation to explain why a certain method exists or to give an
> example, which helps the reader understand what the method does.  But in
> this case I don't think mentioning solib-svr4 helps understand what
> find_solib_ns does.
>
> Simon
>
I'll fix these other comments.

-- 
Cheers,
Guinevere Larsen
She/Her/Hers