Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Kevin Buettner <kevinb@redhat.com>
To: Guinevere Larsen <guinevere@redhat.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH v3] gdb: Introduce user-friendly namespace identifier for "info shared"
Date: Sat, 5 Apr 2025 13:17:31 -0700	[thread overview]
Message-ID: <20250405131731.37898256@f41-zbm-amd> (raw)
In-Reply-To: <20250327171312.2768622-2-guinevere@redhat.com>

On Thu, 27 Mar 2025 14:13:13 -0300
Guinevere Larsen <guinevere@redhat.com> wrote:

> This commit is the first step in improving the user experience around
> multiple namespace support. It introduces a user-friendly identifier for
> namespaces, in the format [[<number>]], that will keep consistent between
> dlmopen and dlclose calls. The plan is for this identifier to be usable
> in expressions like `print [[1]]::var` to find a specific instance of a
> symbol, and so the identifier must not be a valid C++ or Ada namespace
> identifier, otherwise disambiguation becomes a problem. Support for
> those expressions has not been implemented yet, it is only mentioned to
> explain why the identifier looks like this.

This syntax seems okay to me.  (I don't have a better suggestion.)

> This syntax was chosen based on the C attributes, since nothing in GDB
> uses a similar syntax that could confuse users. Other syntax options
> that were explored were "#<number>" and "@<number>". The former was
> abandoned because when printing a frame, the frame number is also
> printed with #<number>, so in a lot of the context in which that the
> identifier would show up, it appears in a confusing way. The latter
> clashes with the array printing syntax, and I believe that the having
> "@N::foo" working completely differently to "foo@2" would also lead to a
> bad user experience.

Thanks for explaining the other options considered.

> The namespace identifiers are stored via a vector inside svr4_info
> object. The vector stores the address of the r_debug objects used by
> glibc to identify each namespace, and the user-friendly ID is the index
> of the r_debug in the vector. This commit also introduces a set storing
> the indices of active namespaces. The glibc I used to develop this patch
> (glibc 2.40 on Fedora 41) doesn't allow an SO to be loaded into a
> deactivated namespace, and requesting a new namespace when a namespace
> was previously closed will reuse that namespace. Because of how this is
> implemented, this patch lets GDB easily track the exact namespace IDs
> that the inferior will see.
> 
> Finally, two new solib_ops function pointers were added, find_solib_ns
> and num_active_namespaces, to allow code outside of solib-svr4 to find
> and use the namespace identifiers and the number of namespaces,
> respectively. As a sanity check, the command `info sharedlibrary` has
> been changed to display the namespace identifier when the inferior has
> more than one active namespace. With this final change, a couple of tests
> had to be tweaked to handle the possible new column, and a new test has
> been created to make sure that the column appears and disappears as
> needed, and that GDB can track the value of the LMID for namespaces.
> 
> ---
> Changes for v3:
> * Changed the namespace identifier display from #N to [[N]]
> 
> Changes for v2:
> * made it so the new column in "info shared" only shows up when multiple
>   namespaces are active
> * changes NEWS and docs to reflect that
> * Added a test for this functionality

Your solib related changes and the new tests look good to me.  The only
(slight) misgiving I have is related to identifier being printed in
the NS column...

(gdb) info sharedlibrary
From                To                  NS    Syms Read   Shared Object Library
0x00007ffff7fc7000  0x00007ffff7fff000  [[1]] Yes         /lib64/ld-linux-x86-64.so.2
0x00007ffff7ea2000  0x00007ffff7f90000  [[0]] Yes (*)     /lib64/libm.so.6
0x00007ffff7caf000  0x00007ffff7ea2000  [[0]] Yes (*)     /lib64/libc.so.6
0x00007ffff7fbb000  0x00007ffff7fbf000  [[1]] Yes         /mesquite2/sourceware-git/f42-review/bld/gdb/testsuite/outputs/gdb.base/dlmopen-ns-ids/dlmopen-lib.so
0x00007ffff7b8f000  0x00007ffff7c7d000  [[1]] Yes (*)     /lib64/libm.so.6
0x00007ffff799c000  0x00007ffff7b8f000  [[1]] Yes (*)     /lib64/libc.so.6
0x00007ffff7fc7000  0x00007ffff7fff000  [[1]] Yes         /lib64/ld-linux-x86-64.so.2
0x00007ffff7fb7000  0x00007ffff7fbb000  [[2]] Yes         /mesquite2/sourceware-git/f42-review/bld/gdb/testsuite/outputs/gdb.base/dlmopen-ns-ids/dlmopen-lib.so
0x00007ffff78ae000  0x00007ffff799c000  [[2]] Yes (*)     /lib64/libm.so.6
0x00007ffff76bb000  0x00007ffff78ae000  [[2]] Yes (*)     /lib64/libc.so.6
0x00007ffff7fc7000  0x00007ffff7fff000  [[1]] Yes         /lib64/ld-linux-x86-64.so.2
0x00007ffff7fb3000  0x00007ffff7fb7000  [[3]] Yes         /mesquite2/sourceware-git/f42-review/bld/gdb/testsuite/outputs/gdb.base/dlmopen-ns-ids/dlmopen-lib.so
0x00007ffff75cd000  0x00007ffff76bb000  [[3]] Yes (*)     /lib64/libm.so.6
0x00007ffff73da000  0x00007ffff75cd000  [[3]] Yes (*)     /lib64/libc.so.6
0x00007ffff7fc7000  0x00007ffff7fff000  [[1]] Yes         /lib64/ld-linux-x86-64.so.2


I'm wondering if it might be better to print just (e.g.) "1" instead
of "[[1]]".  The argument in favor of the way you're doing it is that
it may be a reminder of the syntax that'll (eventually) be used to
specify linker namespaces in expressions.  Since it is kind of odd
and may be difficult to remember, especially since this is such
a niche feature, I think it's okay.

I think this patch should go in so that further progress can be made
on linker namespace support.  Quibbles regarding whether the NS
column should have the square brackets or not can be addressed in
later patches if necessary.

The approval give below assumes that you've fixed the testcase as
mentioned in reply.  (I ran into this and added your fix to my local
sources for testing.  FWIW, I tested on x86_64, aarch64, and riscv.)

Approved-by: Kevin Buettner <kevinb@redhat.com>


  parent reply	other threads:[~2025-04-05 20:18 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-03-13 17:00 [PATCH] " Guinevere Larsen
2025-03-13 19:19 ` Eli Zaretskii
2025-03-13 19:27   ` Guinevere Larsen
2025-03-15  2:51 ` Kevin Buettner
2025-03-15  3:11   ` Kevin Buettner
2025-03-17 11:55     ` Guinevere Larsen
2025-03-17 15:36       ` Simon Marchi
2025-03-18  1:07       ` Kevin Buettner
2025-03-17 15:36 ` Simon Marchi
2025-03-17 17:07   ` Guinevere Larsen
2025-03-17 17:54     ` Simon Marchi
2025-03-19 12:30 ` [PATCH v2] " Guinevere Larsen
2025-03-21 17:55   ` Guinevere Larsen
2025-03-27 17:13   ` [PATCH v3] " Guinevere Larsen
2025-03-31 19:34     ` Guinevere Larsen
2025-04-05 20:17     ` Kevin Buettner [this message]
2025-04-07 13:07       ` Guinevere Larsen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250405131731.37898256@f41-zbm-amd \
    --to=kevinb@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=guinevere@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox