diff mbox series

[RFC,2/3] gdb/nat: Factor linux_find_proc_stat_field out of linux_common_core_of_thread

Message ID 20240321231149.519549-3-thiago.bauermann@linaro.org
State New
Headers show
Series Fix attaching to process when it has zombie threads | expand

Commit Message

Thiago Jung Bauermann March 21, 2024, 11:11 p.m. UTC 
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.

Also, take the opportunity to move the function's documentation comment to
the header file, to conform with GDB practice.
---
 gdb/nat/linux-osdata.c | 43 ++++++++++++++++++++++++++++--------------
 gdb/nat/linux-osdata.h |  3 +++
 2 files changed, 32 insertions(+), 14 deletions(-)

Comments

Luis Machado March 22, 2024, 4:12 p.m. UTC | #1 
On 3/21/24 23:11, 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.
> 
> Also, take the opportunity to move the function's documentation comment to
> the header file, to conform with GDB practice.
> ---
>  gdb/nat/linux-osdata.c | 43 ++++++++++++++++++++++++++++--------------
>  gdb/nat/linux-osdata.h |  3 +++
>  2 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
> index 172fea5cea85..c254f2e4f05b 100644
> --- a/gdb/nat/linux-osdata.c
> +++ b/gdb/nat/linux-osdata.c
> @@ -53,32 +53,31 @@ typedef long long TIME_T;
>  
>  #define MAX_PID_T_STRLEN  (sizeof ("-9223372036854775808") - 1)
>  
> -/* Returns the CPU core that thread PTID is currently running on.  */
> +/* Returns FIELD (as numbered in procfs(5) man page) of
> +   /proc/PID/task/LWP/stat file.  */
>  
> -/* Compute and return the processor core of a given thread.  */
> -
> -int
> -linux_common_core_of_thread (ptid_t ptid)
> +static std::optional<std::string>
> +linux_find_proc_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 >= 3);
> +
>    char filename[sizeof ("/proc//task//stat") + 2 * MAX_PID_T_STRLEN];
> -  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 ())
> -    return -1;
> +    return {};
>  
>    /* 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;
> +    return {};
>  
> -  /* If the first field after program name has index 3, then core number is
> -     the field with index 39.  These are the indexes shown in the procfs(5)
> -     man page.  */
> -  for (int i = 3; i <= 39; ++i)
> +  for (int i = 3; i <= field; ++i)
>      {
>        /* Find separator.  */
>        pos = content->find_first_of (' ', pos);
> @@ -91,8 +90,24 @@ linux_common_core_of_thread (ptid_t ptid)
>  	return {};
>      }
>  
> -  if (sscanf (&(*content)[pos], "%d", &core) == 0)
> -    core = -1;
> +  /* 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-osdata.h.  */
> +
> +int
> +linux_common_core_of_thread (ptid_t ptid)
> +{
> +  std::optional<std::string> field = linux_find_proc_stat_field (ptid, 39);
> +  int core;
> +
> +  if (!field.has_value () || sscanf (field->c_str (), "%d", &core) == 0)
> +    return -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);
>  

Looks OK overall. I'd just attempt to replace the constants (3 and 39) with something more
explicit (or an enum, not sure if we need to go that far though)

Reviewed-By: Luis Machado <luis.machado@arm.com>
Pedro Alves April 17, 2024, 4:06 p.m. UTC | #2 
On 2024-03-21 23:11, 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.
> 
> Also, take the opportunity to move the function's documentation comment to
> the header file, to conform with GDB practice.

IMO, it'd be even better to move the new function to linux-proc.c, with a
linux_proc prefix.
Thiago Jung Bauermann April 20, 2024, 5:16 a.m. UTC | #3 
Pedro Alves <pedro@palves.net> writes:

> On 2024-03-21 23:11, 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.
>>
>> Also, take the opportunity to move the function's documentation comment to
>> the header file, to conform with GDB practice.
>
> IMO, it'd be even better to move the new function to linux-proc.c, with a
> linux_proc prefix.

Done in v2.

--
Thiago
diff mbox series

Patch

diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
index 172fea5cea85..c254f2e4f05b 100644
--- a/gdb/nat/linux-osdata.c
+++ b/gdb/nat/linux-osdata.c
@@ -53,32 +53,31 @@  typedef long long TIME_T;
 
 #define MAX_PID_T_STRLEN  (sizeof ("-9223372036854775808") - 1)
 
-/* Returns the CPU core that thread PTID is currently running on.  */
+/* Returns FIELD (as numbered in procfs(5) man page) of
+   /proc/PID/task/LWP/stat file.  */
 
-/* Compute and return the processor core of a given thread.  */
-
-int
-linux_common_core_of_thread (ptid_t ptid)
+static std::optional<std::string>
+linux_find_proc_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 >= 3);
+
   char filename[sizeof ("/proc//task//stat") + 2 * MAX_PID_T_STRLEN];
-  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 ())
-    return -1;
+    return {};
 
   /* 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;
+    return {};
 
-  /* If the first field after program name has index 3, then core number is
-     the field with index 39.  These are the indexes shown in the procfs(5)
-     man page.  */
-  for (int i = 3; i <= 39; ++i)
+  for (int i = 3; i <= field; ++i)
     {
       /* Find separator.  */
       pos = content->find_first_of (' ', pos);
@@ -91,8 +90,24 @@  linux_common_core_of_thread (ptid_t ptid)
 	return {};
     }
 
-  if (sscanf (&(*content)[pos], "%d", &core) == 0)
-    core = -1;
+  /* 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-osdata.h.  */
+
+int
+linux_common_core_of_thread (ptid_t ptid)
+{
+  std::optional<std::string> field = linux_find_proc_stat_field (ptid, 39);
+  int core;
+
+  if (!field.has_value () || sscanf (field->c_str (), "%d", &core) == 0)
+    return -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);