Re: [PATCH v1 02/10] gdb: rely on the first alive thread TPID when reading Linux procfs files

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Matthieu Longo <matthieu.longo@arm.com> writes:

> On Linux, /proc/<pid> is keyed by the thread-group leader PID. When
> the leader has exited, some /proc/<pid>/... entries become unavailable
> even though another thread is still alive. This can happen, for instance,
> when the main thread calls pthread_exit() and another thread continues
> the execution (existing test: gcore-stale-thread).
>
> This causes GDB to fail to read procfs entries such as cmdline, cwd,
> exe, maps, and smaps when it uses 'current_inferior ()->pid' after the
> thread-group leader has exited.
>
> Fix this by adding inferior::first_alive_thread(), which returns the
> PTID of the first non-exited thread of the inferior. Use its LWP ID
> when accessing procfs entries that only need a representative live LWP
> belonging to the process.
>
> Update linux_info_proc, linux_process_address_in_memtag_page, and
> linux_find_memory_regions_full to use this live-thread LWP ID instead
> of the inferior PID when constructing procfs paths.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31207
> ---
>  gdb/inferior.c   | 12 ++++++++++++
>  gdb/inferior.h   |  9 +++++++++
>  gdb/linux-tdep.c | 38 ++++++++++++++++++--------------------
>  3 files changed, 39 insertions(+), 20 deletions(-)

It's a pity that the patch originally proposted to solve this bug:

https://inbox.sourceware.org/gdb-patches/20250412201140.31510-1-dominik.b.czarnota@gmail.com/

was approved but never committed. But your version is more
comprehensive, since it also makes the change for other procfs files.
It also explicitly looks for a live thread.

I have a couple of comments below, but regardless:

Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index 155d6e874d9..b89f4ee0717 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -842,9 +842,7 @@ static void
>  linux_info_proc (struct gdbarch *gdbarch, const char *args,
>  		 enum info_proc_what what)
>  {
> -  /* A long is used for pid instead of an int to avoid a loss of precision
> -     compiler warning from the output of strtoul.  */
> -  long pid;
> +  ptid_t ptid;
>    int cmdline_f = (what == IP_MINIMAL || what == IP_CMDLINE || what == IP_ALL);
>    int cwd_f = (what == IP_MINIMAL || what == IP_CWD || what == IP_ALL);
>    int environ_f = (what == IP_ENVIRON || what == IP_ALL);
> @@ -859,7 +857,8 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>      {
>        char *tem;
>  
> -      pid = strtoul (args, &tem, 10);
> +      auto pid = strtoul (args, &tem, 10);

The actual type of pid here will be long, right? Unless there's a an
advantage to using auto, I think in this case the code is clearer if the
type is explicitly mentioned.

> +      ptid = ptid_t (pid, pid);
>        args = tem;
>      }
>    else
> @@ -869,17 +868,17 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>        if (current_inferior ()->fake_pid_p)
>  	error (_("Can't determine the current process's PID: you must name one."));
>  
> -      pid = current_inferior ()->pid;
> +      ptid = current_inferior ()->first_alive_thread ();
>      }
>  
>    args = skip_spaces (args);
>    if (args && args[0])
>      error (_("Too many parameters: %s"), args);
>  
> -  gdb_printf (_("process %ld\n"), pid);
> +  gdb_printf (_("process %d\n"), ptid.pid ());
>    if (cmdline_f)
>      {
> -      xsnprintf (filename, sizeof filename, "/proc/%ld/cmdline", pid);
> +      xsnprintf (filename, sizeof filename, "/proc/%ld/cmdline", ptid.lwp ());
>        gdb_byte *buffer;
>        LONGEST len = target_fileio_read_alloc (nullptr, filename, &buffer);
>  
> @@ -901,7 +900,7 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>      }
>    if (cwd_f)
>      {
> -      xsnprintf (filename, sizeof filename, "/proc/%ld/cwd", pid);
> +      xsnprintf (filename, sizeof filename, "/proc/%ld/cwd", ptid.lwp ());

proc_pid_cwd(5) says:

  In a multithreaded process, the contents of this symbolic
  link are not available if the main thread has already
  terminated (typically by calling pthread_exit(3)).

I think it's ok to leave the code as is for consistency, but it's worth
mentioning this detail in a comment.

>        std::optional<std::string> contents
>  	= target_fileio_readlink (NULL, filename, &target_errno);
>        if (contents.has_value ())
> @@ -911,7 +910,7 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>      }
>    if (environ_f)
>      {
> -      xsnprintf (filename, sizeof filename, "/proc/%ld/environ", pid);
> +      xsnprintf (filename, sizeof filename, "/proc/%ld/environ", ptid.lwp ());
>        gdb_byte *buffer;
>        LONGEST len = target_fileio_read_alloc (nullptr, filename, &buffer);
>  
> @@ -935,7 +934,7 @@ linux_info_proc (struct gdbarch *gdbarch, const char *args,
>      }
>    if (exe_f)
>      {
> -      xsnprintf (filename, sizeof filename, "/proc/%ld/exe", pid);
> +      xsnprintf (filename, sizeof filename, "/proc/%ld/exe", ptid.lwp ());

Same comment here. proc_pid_exe(5) has the same observation.

>        std::optional<std::string> contents
>  	= target_fileio_readlink (NULL, filename, &target_errno);
>        if (contents.has_value ())

-- 
Thiago
(he/him)