| Message ID | 20240420055652.819024-2-thiago.bauermann@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Fix attaching to process when it has zombie threads | expand |
On 2024-04-20 06:56, Thiago Jung Bauermann wrote: > diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c > index 6ffabe90aa7d..079f9ea20c37 100644 > --- a/gdb/nat/linux-osdata.c > +++ b/gdb/nat/linux-osdata.c > @@ -52,6 +52,10 @@ 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 > + > + /* 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. */ If you're going to have macros, then the comment shouldn't be repeating the numbers. This is one way I thought it would be worse -- before the numbers were all in the same four lines, there was basically no chance of getting them out of sync. Now they are spread out in two different places. For the logic below, what isn't obvious and deserves a comment is, IMO: - the state field is that one that comes first after the command name field. - we want to get to the "processor" field (not obvious because we call it "core"). > + for (int i = LINUX_PROC_STAT_STATE; i <= LINUX_PROC_STAT_PROCESSOR; ++i) > { > /* Find separator. */ > pos = content->find_first_of (' ', pos);
Hello Pedro, Thank you for the review! Pedro Alves <pedro@palves.net> writes: > On 2024-04-20 06:56, Thiago Jung Bauermann wrote: > >> diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c >> index 6ffabe90aa7d..079f9ea20c37 100644 >> --- a/gdb/nat/linux-osdata.c >> +++ b/gdb/nat/linux-osdata.c >> @@ -52,6 +52,10 @@ 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 >> + > > >> + /* 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. */ > > If you're going to have macros, then the comment shouldn't be repeating the > numbers. This is one way I thought it would be worse -- before the numbers were > all in the same four lines, there was basically no chance of getting them out of > sync. Now they are spread out in two different places. > > For the logic below, what isn't obvious and deserves a comment is, IMO: > > - the state field is that one that comes first after the command name field. > - we want to get to the "processor" field (not obvious because we call it "core"). Yes, good points. I reworded the comment in v3 to address them. >> + for (int i = LINUX_PROC_STAT_STATE; i <= LINUX_PROC_STAT_PROCESSOR; ++i) >> { >> /* Find separator. */ >> pos = content->find_first_of (' ', pos);
diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c index 6ffabe90aa7d..079f9ea20c37 100644 --- a/gdb/nat/linux-osdata.c +++ b/gdb/nat/linux-osdata.c @@ -52,6 +52,10 @@ 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. */ @@ -74,10 +78,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 = LINUX_PROC_STAT_STATE; i <= LINUX_PROC_STAT_PROCESSOR; ++i) { /* Find separator. */ pos = content->find_first_of (' ', pos);
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. Reviewed-By: Luis Machado <luis.machado@arm.com> --- gdb/nat/linux-osdata.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) Changes in v2: - Added macros for field indexes in /proc/PID/stat (Suggested by Luis).