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

[Simon Marchi <simark@simark.ca>]

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.

> 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