diff mbox series

[RFC,1/3] gdb/nat: Use procfs(5) indexes in linux_common_core_of_thread

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

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Thiago Jung Bauermann March 21, 2024, 11:11 p.m. UTC 
  The code and comment reference stat fields by made-up indexes.  The
procfs(5) man page, which describes the /proc/PID/stat file, has a numbered
list of these fields so it's more convenient to use those numbers instead.

This is currently an implementation detail inside the function so it's
not really relevant with the code as-is, but a future patch will do some
refactoring which will make the index more prominent.

Therefore, make this change in a separate patch so that it's simpler to
review.
---
 gdb/nat/linux-osdata.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Luis Machado March 22, 2024, 5:33 p.m. UTC | #1 
On 3/21/24 23:11, Thiago Jung Bauermann wrote:
> The code and comment reference stat fields by made-up indexes.  The
> procfs(5) man page, which describes the /proc/PID/stat file, has a numbered
> list of these fields so it's more convenient to use those numbers instead.
> 
> This is currently an implementation detail inside the function so it's
> not really relevant with the code as-is, but a future patch will do some
> refactoring which will make the index more prominent.
> 
> Therefore, make this change in a separate patch so that it's simpler to
> review.
> ---
>  gdb/nat/linux-osdata.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
> index c9192940f236..172fea5cea85 100644
> --- a/gdb/nat/linux-osdata.c
> +++ b/gdb/nat/linux-osdata.c
> @@ -75,10 +75,10 @@ linux_common_core_of_thread (ptid_t ptid)
>    if (pos == std::string::npos)
>      return -1;
>  
> -  /* If the first field after program name has index 0, then core number is
> -     the field with index 36 (so, the 37th).  There's no constant for that
> -     anywhere.  */
> -  for (int i = 0; i < 37; ++i)
> +  /* 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)
>      {
>        /* Find separator.  */
>        pos = content->find_first_of (' ', pos);

Looks ok to me.

See the comment about turning the numeric constants into named constants.

Reviewed-By: Luis Machado <luis.machado@arm.com>
Pedro Alves April 17, 2024, 3:55 p.m. UTC | #2 
On 2024-03-22 17:33, Luis Machado wrote:

>> ---
>>  gdb/nat/linux-osdata.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
>> index c9192940f236..172fea5cea85 100644
>> --- a/gdb/nat/linux-osdata.c
>> +++ b/gdb/nat/linux-osdata.c
>> @@ -75,10 +75,10 @@ linux_common_core_of_thread (ptid_t ptid)
>>    if (pos == std::string::npos)
>>      return -1;
>>  
>> -  /* If the first field after program name has index 0, then core number is
>> -     the field with index 36 (so, the 37th).  There's no constant for that
>> -     anywhere.  */
>> -  for (int i = 0; i < 37; ++i)
>> +  /* 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)
>>      {
>>        /* Find separator.  */
>>        pos = content->find_first_of (' ', pos);
> 
> Looks ok to me.
> 
> See the comment about turning the numeric constants into named constants.

IMHO, macros here would obfuscate more than help.
Thiago Jung Bauermann April 20, 2024, 5:15 a.m. UTC | #3 
Pedro Alves <pedro@palves.net> writes:

> On 2024-03-22 17:33, Luis Machado wrote:
>
>>> ---
>>>  gdb/nat/linux-osdata.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
>>> index c9192940f236..172fea5cea85 100644
>>> --- a/gdb/nat/linux-osdata.c
>>> +++ b/gdb/nat/linux-osdata.c
>>> @@ -75,10 +75,10 @@ linux_common_core_of_thread (ptid_t ptid)
>>>    if (pos == std::string::npos)
>>>      return -1;
>>>
>>> -  /* If the first field after program name has index 0, then core number is
>>> -     the field with index 36 (so, the 37th).  There's no constant for that
>>> -     anywhere.  */
>>> -  for (int i = 0; i < 37; ++i)
>>> +  /* 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)
>>>      {
>>>        /* Find separator.  */
>>>        pos = content->find_first_of (' ', pos);
>>
>> Looks ok to me.
>>
>> See the comment about turning the numeric constants into named constants.
>
> IMHO, macros here would obfuscate more than help.

In v2, I'm sending with the macros so that it's easy to compare and
decide. I can send a v3 without macros again if needed.

--
Thiago
diff mbox series

Patch

diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c
index c9192940f236..172fea5cea85 100644
--- a/gdb/nat/linux-osdata.c
+++ b/gdb/nat/linux-osdata.c
@@ -75,10 +75,10 @@  linux_common_core_of_thread (ptid_t ptid)
   if (pos == std::string::npos)
     return -1;
 
-  /* If the first field after program name has index 0, then core number is
-     the field with index 36 (so, the 37th).  There's no constant for that
-     anywhere.  */
-  for (int i = 0; i < 37; ++i)
+  /* 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)
     {
       /* Find separator.  */
       pos = content->find_first_of (' ', pos);