[PATCH v2] gdb: print thread names in thread apply command output

[PATCH v2] gdb: print thread names in thread apply command output

[Jérémie Galarneau]

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".

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..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
-- 
2.25.1

Re: [PATCH v2] gdb: print thread names in thread apply command output

[Tom Tromey]

>>>>> "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

Re: [PATCH v2] gdb: print thread names in thread apply command output

[Jérémie Galarneau]

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



-- 
Jérémie Galarneau
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH v2] gdb: print thread names in thread apply command output

[Simon Marchi]

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

Re: [PATCH v2] gdb: print thread names in thread apply command output

[Simon Marchi]

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