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 09:59, Luis Machado wrote:
> 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.
> 

Eh, or v4 rather. :-)

>> +  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.