Re: [PATCH v3 2/3] gdb/nat: Factor linux_proc_get_stat_field out of linux_common_core_of_thread

[Luis Machado <luis.machado@arm.com>]

On 4/25/24 00:15, Thiago Jung Bauermann wrote:
> The new function will be used in a subsequent patch to read a different
> stat field.
> 
> The new code is believed to be equivalent to the old code, so there
> should be no change in GDB behaviour.  The only material change was to
> use std::string and string_printf rather than a fixed char array to
> build the path to the stat file.
> 
> Also, take the opportunity to move the function's documentation comment to
> the header file, to conform with GDB practice.
> 
> Reviewed-By: Luis Machado <luis.machado@arm.com>
> ---
>  gdb/nat/linux-osdata.c | 41 +++++----------------------------------
>  gdb/nat/linux-osdata.h |  3 +++
>  gdb/nat/linux-procfs.c | 44 ++++++++++++++++++++++++++++++++++++++++++
>  gdb/nat/linux-procfs.h | 10 ++++++++++
>  4 files changed, 62 insertions(+), 36 deletions(-)
> 
> Changes in v3:
> - Preserve comment mentioning why we use that field in the for loop in
>   linux_proc_get_stat_field.
> 
> Changes in v2:
> - Added macros for field indexes in /proc/PID/stat (Suggested by Luis).
> - Moved linux_find_proc_stat_field from linux-osdata.c to linux-procfs.c
>   and changed prefix to linux_proc (Suggested by Pedro).
> - Use string_printf in linux_proc_get_stat_field to build path to stat
>   file, to avoid having to copy the PID_T and MAX_PID_T_STRLEN macros.
> 
> diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
> index 4812bc735e86..3a6215015f12 100644
> --- a/gdb/nat/linux-osdata.c
> +++ b/gdb/nat/linux-osdata.c
> @@ -36,6 +36,7 @@
>  #include <sys/stat.h>
>  #include "gdbsupport/filestuff.h"
>  #include <algorithm>
> +#include "linux-procfs.h"
>  
>  #define NAMELEN(dirent) strlen ((dirent)->d_name)
>  
> @@ -52,50 +53,18 @@ typedef long long TIME_T;
>  
>  #define MAX_PID_T_STRLEN  (sizeof ("-9223372036854775808") - 1)
>  
> -/* Index of fields of interest in /proc/PID/stat, from procfs(5) man page.  */
> -#define LINUX_PROC_STAT_STATE 3
> -#define LINUX_PROC_STAT_PROCESSOR 39
> -
> -/* Returns the CPU core that thread PTID is currently running on.  */
> -
> -/* Compute and return the processor core of a given thread.  */
> +/* See linux-osdata.h.  */
>  
>  int
>  linux_common_core_of_thread (ptid_t ptid)
>  {
> -  char filename[sizeof ("/proc//task//stat") + 2 * MAX_PID_T_STRLEN];
> +  std::optional<std::string> field
> +    = linux_proc_get_stat_field (ptid, LINUX_PROC_STAT_PROCESSOR);
>    int core;
>  
> -  sprintf (filename, "/proc/%lld/task/%lld/stat",
> -	   (PID_T) ptid.pid (), (PID_T) ptid.lwp ());
> -
> -  std::optional<std::string> content = read_text_file_to_string (filename);
> -  if (!content.has_value ())
> +  if (!field.has_value () || sscanf (field->c_str (), "%d", &core) == 0)
>      return -1;
>  
> -  /* ps command also relies on no trailing fields ever contain ')'.  */
> -  std::string::size_type pos = content->find_last_of (')');
> -  if (pos == std::string::npos)
> -    return -1;
> -
> -  /* The first field after program name is LINUX_PROC_STAT_STATE, and we are
> -     interested in field LINUX_PROC_STAT_PROCESSOR.  */
> -  for (int i = LINUX_PROC_STAT_STATE; i <= LINUX_PROC_STAT_PROCESSOR; ++i)
> -    {
> -      /* Find separator.  */
> -      pos = content->find_first_of (' ', pos);
> -      if (pos == std::string::npos)
> -	return {};
> -
> -      /* Find beginning of field.  */
> -      pos = content->find_first_not_of (' ', pos);
> -      if (pos == std::string::npos)
> -	return {};
> -    }
> -
> -  if (sscanf (&(*content)[pos], "%d", &core) == 0)
> -    core = -1;
> -
>    return core;
>  }
>  
> diff --git a/gdb/nat/linux-osdata.h b/gdb/nat/linux-osdata.h
> index 833915cdb2fd..a82fb08b998e 100644
> --- a/gdb/nat/linux-osdata.h
> +++ b/gdb/nat/linux-osdata.h
> @@ -20,7 +20,10 @@
>  #ifndef NAT_LINUX_OSDATA_H
>  #define NAT_LINUX_OSDATA_H
>  
> +/* Returns the CPU core that thread PTID is currently running on.  */
> +
>  extern int linux_common_core_of_thread (ptid_t ptid);
> +
>  extern LONGEST linux_common_xfer_osdata (const char *annex, gdb_byte *readbuf,
>  					 ULONGEST offset, ULONGEST len);
>  
> diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
> index e2086952ce6b..c11eaf3cc6fd 100644
> --- a/gdb/nat/linux-procfs.c
> +++ b/gdb/nat/linux-procfs.c
> @@ -230,6 +230,50 @@ linux_proc_pid_is_zombie (pid_t pid)
>  
>  /* See linux-procfs.h.  */
>  
> +std::optional<std::string>
> +linux_proc_get_stat_field (ptid_t ptid, int field)
> +{
> +  /* We never need to read PID from the stat file, and there's
> +     command_from_pid to read the comm field.  */
> +  gdb_assert (field >= LINUX_PROC_STAT_STATE);
> +
> +  std::string filename = string_printf ("/proc/%ld/task/%ld/stat",
> +					(long) ptid.pid (), (long) ptid.lwp ());
> +
> +  std::optional<std::string> content
> +    = read_text_file_to_string (filename.c_str ());
> +  if (!content.has_value ())
> +    return {};
> +
> +  /* ps command also relies on no trailing fields ever contain ')'.  */

Probably an existing typo, but s/contain/containing?

No need to send a v5 for this though.

> +  std::string::size_type pos = content->find_last_of (')');
> +  if (pos == std::string::npos)
> +    return {};
> +
> +  /* The first field after program name is LINUX_PROC_STAT_STATE.  */
> +  for (int i = LINUX_PROC_STAT_STATE; i <= field; ++i)
> +    {
> +      /* Find separator.  */
> +      pos = content->find_first_of (' ', pos);
> +      if (pos == std::string::npos)
> +	return {};
> +
> +      /* Find beginning of field.  */
> +      pos = content->find_first_not_of (' ', pos);
> +      if (pos == std::string::npos)
> +	return {};
> +    }
> +
> +  /* Find end of field.  */
> +  std::string::size_type end_pos = content->find_first_of (' ', pos);
> +  if (end_pos == std::string::npos)
> +    return content->substr (pos);
> +  else
> +    return content->substr (pos, end_pos - pos);
> +}
> +
> +/* See linux-procfs.h.  */
> +
>  const char *
>  linux_proc_tid_get_name (ptid_t ptid)
>  {
> diff --git a/gdb/nat/linux-procfs.h b/gdb/nat/linux-procfs.h
> index 880afbcdd615..ec1f37651fbf 100644
> --- a/gdb/nat/linux-procfs.h
> +++ b/gdb/nat/linux-procfs.h
> @@ -54,6 +54,16 @@ extern int linux_proc_pid_is_zombie_nowarn (pid_t pid);
>  
>  extern int linux_proc_pid_is_gone (pid_t pid);
>  
> +/* Index of fields of interest in /proc/PID/stat, from procfs(5) man page.  */
> +#define LINUX_PROC_STAT_STATE 3
> +#define LINUX_PROC_STAT_PROCESSOR 39
> +
> +/* Returns FIELD (as numbered in procfs(5) man page) of
> +   /proc/PID/task/LWP/stat file.  */
> +
> +extern std::optional<std::string> linux_proc_get_stat_field (ptid_t ptid,
> +							     int field);
> +
>  /* Return a string giving the thread's name or NULL if the
>     information is unavailable.  The returned value points to a statically
>     allocated buffer.  The value therefore becomes invalid at the next

Otherwise looks OK.