Message ID | 20200217223021.38030-1-jeremie.galarneau@efficios.com |
---|---|
State | New |
Headers | show |
Hi, Thanks for the patch. Some comments below. On 2/17/20 7:30 PM, Jérémie Galarneau wrote: > This makes the thread apply command print the thread's name. The use > of target_pid_to_str is replaced by thread_target_id_str, which > provides the same output as "info threads". Is this a cosmetic change to the way GDB outputs things? If so, I wonder if the current tests expect the old format and thus will start to fail with the new output? Have you exercised the testsuite? > > Before: > (gdb) thread apply 2 bt > > Thread 2 (Thread 0x7fd245602700 (LWP 3837)): > [...] > > After: > (gdb) thread apply 2 bt > > Thread 2 (Thread 0x7fd245602700 (LWP 3837) "HT cleanup"): > [...] > > The thread's description header is pre-computed before running the > command since the command may change the selected inferior. This is > not permitted by thread_target_id_str as target_thread_name asserts > that `info->inf == current_inferior ()`. > > This situation arises in the `gdb.threads/threadapply.exp` test which > kills and removes the inferior as part of a "thread apply" command. > > gdb/ChangeLog: > > * thread.c (thr_try_catch_cmd): Print thread name. > --- > gdb/ChangeLog | 4 ++++ > gdb/thread.c | 16 ++++++++++------ > 2 files changed, 14 insertions(+), 6 deletions(-) > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > index dafd90ec37..4fce196187 100644 > --- a/gdb/ChangeLog > +++ b/gdb/ChangeLog > @@ -1,3 +1,7 @@ > +2020-02-17 Jérémie Galarneau <jeremie.galarneau@efficios.com> > + > + * thread.c (thr_try_catch_cmd): Print thread name. > + > 2020-02-14 Simon Marchi <simon.marchi@efficios.com> > > * aarch64-tdep.c (aarch64_displaced_step_copy_insn): Use > diff --git a/gdb/thread.c b/gdb/thread.c > index 54b59e2244..0c1611bdc1 100644 > --- a/gdb/thread.c > +++ b/gdb/thread.c > @@ -1563,6 +1563,14 @@ thr_try_catch_cmd (thread_info *thr, const char *cmd, int from_tty, > const qcs_flags &flags) > { > switch_to_thread (thr); > + > + /* The thread header is computed before running the command since > + * the command can change the inferior, which is not permitted > + * by thread_target_id_str. */ The formatting of the comment block is not right. We don't start intermediate lines with *. You can check existing blocks for the format that we use. > + std::string thr_header = string_printf(_("\nThread %s (%s):\n"), > + print_thread_id (thr), > + thread_target_id_str (thr).c_str ()); Maybe write it as: std::string thr_header = string_printf(_("\nThread %s (%s):\n"), print_thread_id (thr), thread_target_id_str (thr).c_str ()); I think it gives it a better alignment. Just a suggestion. > + > try > { > std::string cmd_result = execute_command_to_string > @@ -1570,9 +1578,7 @@ thr_try_catch_cmd (thread_info *thr, const char *cmd, int from_tty, > if (!flags.silent || cmd_result.length () > 0) > { > if (!flags.quiet) > - printf_filtered (_("\nThread %s (%s):\n"), > - print_thread_id (thr), > - target_pid_to_str (inferior_ptid).c_str ()); > + printf_filtered ("%s", thr_header.c_str ()); > printf_filtered ("%s", cmd_result.c_str ()); > } > } > @@ -1581,9 +1587,7 @@ thr_try_catch_cmd (thread_info *thr, const char *cmd, int from_tty, > if (!flags.silent) > { > if (!flags.quiet) > - printf_filtered (_("\nThread %s (%s):\n"), > - print_thread_id (thr), > - target_pid_to_str (inferior_ptid).c_str ()); > + printf_filtered ("%s", thr_header.c_str ()); > if (flags.cont) > printf_filtered ("%s\n", ex.what ()); > else >
On Mon, 17 Feb 2020 at 18:17, Luis Machado <luis.machado@linaro.org> wrote: > > Hi, > > Thanks for the patch. Some comments below. > > On 2/17/20 7:30 PM, Jérémie Galarneau wrote: > > This makes the thread apply command print the thread's name. The use > > of target_pid_to_str is replaced by thread_target_id_str, which > > provides the same output as "info threads". > > Is this a cosmetic change to the way GDB outputs things? If so, I wonder > if the current tests expect the old format and thus will start to fail > with the new output? Have you exercised the testsuite? Hi, It does indeed introduce a cosmetic change. I have exercised the test suite with and without my patch applied. The only difference I see is "gdb.threads/process-dies-while-handling-bp.exp" which sometimes passes/fails, but I think this test is known to be flaky. > > > > > Before: > > (gdb) thread apply 2 bt > > > > Thread 2 (Thread 0x7fd245602700 (LWP 3837)): > > [...] > > > > After: > > (gdb) thread apply 2 bt > > > > Thread 2 (Thread 0x7fd245602700 (LWP 3837) "HT cleanup"): > > [...] > > > > The thread's description header is pre-computed before running the > > command since the command may change the selected inferior. This is > > not permitted by thread_target_id_str as target_thread_name asserts > > that `info->inf == current_inferior ()`. > > > > This situation arises in the `gdb.threads/threadapply.exp` test which > > kills and removes the inferior as part of a "thread apply" command. > > > > gdb/ChangeLog: > > > > * thread.c (thr_try_catch_cmd): Print thread name. > > --- > > gdb/ChangeLog | 4 ++++ > > gdb/thread.c | 16 ++++++++++------ > > 2 files changed, 14 insertions(+), 6 deletions(-) > > > > diff --git a/gdb/ChangeLog b/gdb/ChangeLog > > index dafd90ec37..4fce196187 100644 > > --- a/gdb/ChangeLog > > +++ b/gdb/ChangeLog > > @@ -1,3 +1,7 @@ > > +2020-02-17 Jérémie Galarneau <jeremie.galarneau@efficios.com> > > + > > + * thread.c (thr_try_catch_cmd): Print thread name. > > + > > 2020-02-14 Simon Marchi <simon.marchi@efficios.com> > > > > * aarch64-tdep.c (aarch64_displaced_step_copy_insn): Use > > diff --git a/gdb/thread.c b/gdb/thread.c > > index 54b59e2244..0c1611bdc1 100644 > > --- a/gdb/thread.c > > +++ b/gdb/thread.c > > @@ -1563,6 +1563,14 @@ thr_try_catch_cmd (thread_info *thr, const char *cmd, int from_tty, > > const qcs_flags &flags) > > { > > switch_to_thread (thr); > > + > > + /* The thread header is computed before running the command since > > + * the command can change the inferior, which is not permitted > > + * by thread_target_id_str. */ > > The formatting of the comment block is not right. We don't start > intermediate lines with *. You can check existing blocks for the format > that we use. Right! Sorry about that. > > > + std::string thr_header = string_printf(_("\nThread %s (%s):\n"), > > + print_thread_id (thr), > > + thread_target_id_str (thr).c_str ()); > > Maybe write it as: > > std::string thr_header = > string_printf(_("\nThread %s (%s):\n"), print_thread_id (thr), > thread_target_id_str (thr).c_str ()); > > I think it gives it a better alignment. Just a suggestion. Alright, I'll send a v2 with those comments addressed. Thanks for the review! Jérémie > > > + > > try > > { > > std::string cmd_result = execute_command_to_string > > @@ -1570,9 +1578,7 @@ thr_try_catch_cmd (thread_info *thr, const char *cmd, int from_tty, > > if (!flags.silent || cmd_result.length () > 0) > > { > > if (!flags.quiet) > > - printf_filtered (_("\nThread %s (%s):\n"), > > - print_thread_id (thr), > > - target_pid_to_str (inferior_ptid).c_str ()); > > + printf_filtered ("%s", thr_header.c_str ()); > > printf_filtered ("%s", cmd_result.c_str ()); > > } > > } > > @@ -1581,9 +1587,7 @@ thr_try_catch_cmd (thread_info *thr, const char *cmd, int from_tty, > > if (!flags.silent) > > { > > if (!flags.quiet) > > - printf_filtered (_("\nThread %s (%s):\n"), > > - print_thread_id (thr), > > - target_pid_to_str (inferior_ptid).c_str ()); > > + printf_filtered ("%s", thr_header.c_str ()); > > if (flags.cont) > > printf_filtered ("%s\n", ex.what ()); > > else > >
On 2020-02-17 7:01 p.m., Jérémie Galarneau wrote: >>> + std::string thr_header = string_printf(_("\nThread %s (%s):\n"), >>> + print_thread_id (thr), >>> + thread_target_id_str (thr).c_str ()); >> >> Maybe write it as: >> >> std::string thr_header = >> string_printf(_("\nThread %s (%s):\n"), print_thread_id (thr), >> thread_target_id_str (thr).c_str ()); >> >> I think it gives it a better alignment. Just a suggestion. > > Alright, I'll send a v2 with those comments addressed. Actually, the GNU styles breaks lines before binary operators, including the assignment operator. See this for example: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/thread.c;h=54b59e22448a260e4190ffe5438b8b5c799a1cd9;hb=HEAD#l1888 No need to send another version just for that, we can fix it up when we push the patch. Simon
diff --git a/gdb/ChangeLog b/gdb/ChangeLog index dafd90ec37..4fce196187 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,7 @@ +2020-02-17 Jérémie Galarneau <jeremie.galarneau@efficios.com> + + * thread.c (thr_try_catch_cmd): Print thread name. + 2020-02-14 Simon Marchi <simon.marchi@efficios.com> * aarch64-tdep.c (aarch64_displaced_step_copy_insn): Use diff --git a/gdb/thread.c b/gdb/thread.c index 54b59e2244..0c1611bdc1 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1563,6 +1563,14 @@ thr_try_catch_cmd (thread_info *thr, const char *cmd, int from_tty, const qcs_flags &flags) { switch_to_thread (thr); + + /* The thread header is computed before running the command since + * the command can change the inferior, which is not permitted + * by thread_target_id_str. */ + std::string thr_header = string_printf(_("\nThread %s (%s):\n"), + print_thread_id (thr), + thread_target_id_str (thr).c_str ()); + try { std::string cmd_result = execute_command_to_string @@ -1570,9 +1578,7 @@ thr_try_catch_cmd (thread_info *thr, const char *cmd, int from_tty, if (!flags.silent || cmd_result.length () > 0) { if (!flags.quiet) - printf_filtered (_("\nThread %s (%s):\n"), - print_thread_id (thr), - target_pid_to_str (inferior_ptid).c_str ()); + printf_filtered ("%s", thr_header.c_str ()); printf_filtered ("%s", cmd_result.c_str ()); } } @@ -1581,9 +1587,7 @@ thr_try_catch_cmd (thread_info *thr, const char *cmd, int from_tty, if (!flags.silent) { if (!flags.quiet) - printf_filtered (_("\nThread %s (%s):\n"), - print_thread_id (thr), - target_pid_to_str (inferior_ptid).c_str ()); + printf_filtered ("%s", thr_header.c_str ()); if (flags.cont) printf_filtered ("%s\n", ex.what ()); else