Don't paginate "new thread" events

Don't paginate "new thread" events

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]

New thread events printing ("New Thread 0x40077950 (LWP 13229)]") was
recently centralised in add_thread.  Unfortunatelly it brought with
it a behaviour change.  Before, we wouldn't paginate when these
events filled the terminal screen, and now we do.  This is quite bad,
as gdb now waits for use input to proceed.  Also, since add_thread
doesn't target_terminal_ours, gdb just stops due to SIGTTOU
when the pagination kicks in ("want to continue y or n" prompt).  User
now has to fg to restore it...

Easilly reproduced by running manythreads.exp manually.

This patch restores the old behaviour.

OK?

-- 
Pedro Alves

[-- Attachment #2: dont_paginate_threads.diff --]
[-- Type: text/x-diff, Size: 689 bytes --]

2008-03-12  Pedro Alves  <pedro@codesourcery.com>

	* thread.c (add_thread): Use printf_unfiltered to print.

---
 gdb/thread.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: src/gdb/thread.c
===================================================================
--- src.orig/gdb/thread.c	2008-03-12 01:36:24.000000000 +0000
+++ src/gdb/thread.c	2008-03-12 01:36:42.000000000 +0000
@@ -136,7 +136,7 @@ add_thread (ptid_t ptid)
   struct thread_info *result = add_thread_silent (ptid);
 
   if (print_thread_events)
-    printf_filtered (_("[New %s]\n"), target_pid_to_str (ptid));
+    printf_unfiltered (_("[New %s]\n"), target_pid_to_str (ptid));
   
   return result;
 }

Re: Don't paginate "new thread" events

[Joel Brobecker]

> New thread events printing ("New Thread 0x40077950 (LWP 13229)]") was
> recently centralised in add_thread.  Unfortunatelly it brought with
> it a behaviour change.  Before, we wouldn't paginate when these
> events filled the terminal screen, and now we do.

Actually, I think it's a little more complicated than that. I had
a second look at the patch, and the routines that were used to print
the event notifications were either "printf_filtered" or "ui_out_text"
(which in CLI mode results in a printf_filtered).

I think the centralization just caused more messages to printed
compared to before, but that's just a guess. Perhaps, you could
check an older debugger (before the notification was centralized
inside "add_thread", for instance 6.7.1) with a much smaller terminal
height, and see if you get the same kind of issues...

> This is quite bad, as gdb now waits for use input to proceed.  Also,
> since add_thread doesn't target_terminal_ours, gdb just stops due to
> SIGTTOU when the pagination kicks in ("want to continue y or n"
> prompt).  User now has to fg to restore it...

However, I tend to agree that having these notifications be
paginated can be annoying. I support the change below, but
I'd like to have other's opinion. Based on the analysis above,
your patch would be introducing a change of behavior, so a
consensus on its usefulness would be nice...

-- 
Joel

Re: Don't paginate "new thread" events

[Daniel Jacobowitz]

On Tue, Mar 11, 2008 at 08:01:29PM -0700, Joel Brobecker wrote:
> > New thread events printing ("New Thread 0x40077950 (LWP 13229)]") was
> > recently centralised in add_thread.  Unfortunatelly it brought with
> > it a behaviour change.  Before, we wouldn't paginate when these
> > events filled the terminal screen, and now we do.
> 
> Actually, I think it's a little more complicated than that. I had
> a second look at the patch, and the routines that were used to print
> the event notifications were either "printf_filtered" or "ui_out_text"
> (which in CLI mode results in a printf_filtered).

The one most frequently encountered, in linux-thread-db.c,
was already unfiltered for this very reason.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Don't paginate "new thread" events

[Joel Brobecker]

> The one most frequently encountered, in linux-thread-db.c,
> was already unfiltered for this very reason.

Ooops, right! A grep instead of a visual read of Vladimir's patch
shows very clearly that there were already 3 locations where we used
to use the printf unfiltered. So Pedro's patch is definitely OK.

2008-03-12  Pedro Alves  <pedro@codesourcery.com>

	* thread.c (add_thread): Use printf_unfiltered to print.

This should go to the branch as well.


---
 gdb/thread.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: src/gdb/thread.c
===================================================================
--- src.orig/gdb/thread.c	2008-03-12 01:36:24.000000000 +0000
+++ src/gdb/thread.c	2008-03-12 01:36:42.000000000 +0000
@@ -136,7 +136,7 @@ add_thread (ptid_t ptid)
   struct thread_info *result = add_thread_silent (ptid);
 
   if (print_thread_events)
-    printf_filtered (_("[New %s]\n"), target_pid_to_str (ptid));
+    printf_unfiltered (_("[New %s]\n"), target_pid_to_str (ptid));
   
   return result;
 }

-- 
Joel

Re: Don't paginate "new thread" events

[Pedro Alves]

A Wednesday 12 March 2008 16:03:06, Joel Brobecker wrote:
> Ooops, right! A grep instead of a visual read of Vladimir's patch
> shows very clearly that there were already 3 locations where we used
> to use the printf unfiltered. So Pedro's patch is definitely OK.
>

Thanks Joel.

> 2008-03-12  Pedro Alves  <pedro@codesourcery.com>
>
> 	* thread.c (add_thread): Use printf_unfiltered to print.
>
> This should go to the branch as well.
>

Checked in to head and branch.

-- 
Pedro Alves

Re: Don't paginate "new thread" events

[Pedro Alves]

A Wednesday 12 March 2008 03:01:29, Joel Brobecker wrote:

> I think the centralization just caused more messages to printed
> compared to before, but that's just a guess. Perhaps, you could
> check an older debugger (before the notification was centralized
> inside "add_thread", for instance 6.7.1) with a much smaller terminal
> height, and see if you get the same kind of issues...
>

I did check with an old debugger, but both systems I have access to,
linux and cygwin, used printf_unfiltered before.  I can only
expect that on the other systems, the user is getting a SIGTTOU,
and nobody has bothered to complain.

> However, I tend to agree that having these notifications be
> paginated can be annoying. I support the change below, but
> I'd like to have other's opinion. Based on the analysis above,
> your patch would be introducing a change of behavior, so a
> consensus on its usefulness would be nice...

Thanks for taking a look!

-- 
Pedro Alves