Message ID | 20200218000803.132171-1-jeremie.galarneau@efficios.com |
---|---|
State | New |
Headers | show |
>>>>> "Jérémie" == Jérémie Galarneau <jeremie.galarneau@efficios.com> writes:
Jérémie> This situation arises in the `gdb.threads/threadapply.exp` test which
Jérémie> kills and removes the inferior as part of a "thread apply" command.
Thank you for the patch.
Jérémie> + /* The thread header is computed before running the command since
Jérémie> + the command can change the inferior, which is not permitted
Jérémie> + by thread_target_id_str. */
Jérémie> + std::string thr_header =
Jérémie> + string_printf(_("\nThread %s (%s):\n"), print_thread_id (thr),
Jérémie> + thread_target_id_str (thr).c_str ());
Missing a space before the "(" after string_printf here.
IIUC this is fixing a latent bug, is that correct?
Anyway this looks ok to me with that nit fixed.
Tom
On Tue, 18 Feb 2020 at 15:57, Tom Tromey <tom@tromey.com> wrote: > > >>>>> "Jérémie" == Jérémie Galarneau <jeremie.galarneau@efficios.com> writes: > > Jérémie> This situation arises in the `gdb.threads/threadapply.exp` test which > Jérémie> kills and removes the inferior as part of a "thread apply" command. > > Thank you for the patch. > > Jérémie> + /* The thread header is computed before running the command since > Jérémie> + the command can change the inferior, which is not permitted > Jérémie> + by thread_target_id_str. */ > Jérémie> + std::string thr_header = > Jérémie> + string_printf(_("\nThread %s (%s):\n"), print_thread_id (thr), > Jérémie> + thread_target_id_str (thr).c_str ()); > > Missing a space before the "(" after string_printf here. > > IIUC this is fixing a latent bug, is that correct? Hi Tom, This is not a bug fix. AFAIK, this information has never been provided as part of that command's output. Thanks for the comments! Jérémie > > Anyway this looks ok to me with that nit fixed. > > Tom
On 2020-02-18 11:24 p.m., Jérémie Galarneau wrote: >> IIUC this is fixing a latent bug, is that correct? > > Hi Tom, > > This is not a bug fix. AFAIK, this information has never been provided > as part of that command's output. I think that Tom is talking about the fact that you moved computing the thread header before calling the command. Despite not causing an assertion, it was technically not correct. Prior to this patch, we compute the thread label using: std::string target_pid_to_str (ptid_t ptid) { return current_top_target ()->pid_to_str (ptid); } Where current_top_target is implemented as: target_ops * current_top_target () { return current_inferior ()->top_target (); } So as you can see, calling target_pid_to_str implicitly relies on the current inferior (for historical and multi-target reasons that would be too long to explain this late) to obtain the target to ask to format the ptid. If the executed command switches the current inferior, it could switch to an inferior of a different target. So we would format the ptid of a thread of target A using the pid_to_str method of target B. Changing the code to using thread_target_id_str caused us to call target_thread_name, which luckily has this assertion: gdb_assert (info->inf == current_inferior ()); I suppose this assertion should not be necessary normally, as we should be able to call the thread_name method using a target inferred from the thread: info->inf->top_target ()->thread_name (info) ... and not care about the current inferior/target. However, I presume the assertion was put there as part of the multi-target work because some implementations of the thread_name target method might implicitly rely in the current inferior being set. So until we audit all these implementations, it has to stay true. Simon
On 2020-02-18 11:24 p.m., Jérémie Galarneau wrote: > On Tue, 18 Feb 2020 at 15:57, Tom Tromey <tom@tromey.com> wrote: >> >>>>>>> "Jérémie" == Jérémie Galarneau <jeremie.galarneau@efficios.com> writes: >> >> Jérémie> This situation arises in the `gdb.threads/threadapply.exp` test which >> Jérémie> kills and removes the inferior as part of a "thread apply" command. >> >> Thank you for the patch. >> >> Jérémie> + /* The thread header is computed before running the command since >> Jérémie> + the command can change the inferior, which is not permitted >> Jérémie> + by thread_target_id_str. */ >> Jérémie> + std::string thr_header = >> Jérémie> + string_printf(_("\nThread %s (%s):\n"), print_thread_id (thr), >> Jérémie> + thread_target_id_str (thr).c_str ()); >> >> Missing a space before the "(" after string_printf here. >> >> IIUC this is fixing a latent bug, is that correct? > > Hi Tom, > > This is not a bug fix. AFAIK, this information has never been provided > as part of that command's output. > > Thanks for the comments! > Jérémie > >> >> Anyway this looks ok to me with that nit fixed. >> >> Tom FYI, I pushed it with that fixed. 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..e581cb83b5 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