[RFC] -thread-select double print stack frame

[RFC] -thread-select double print stack frame

[Denis PILAT]

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

Following discussion with Nick 
(http://sources.redhat.com/ml/gdb-patches/2007-03/msg00156.html),
about mi output of -thread-info new command,
here is a patch that fix a problem with *-thread-select* that double 
print the stack frame.

Example:
-thread-select 1
^done,new-thread-id="1",frame={level="0",func="done_making_threads",args=[],file="/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/testsuite/gdb.mi/pthreads.c",fullname="/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/testsuite/gdb.mi/pthreads.c",line="61"},line="61",file="/project/flexdbug/user/pd10/gdb/sts-gdb-repository/vendor/cvs/head/src/gdb/testsuite/gdb.mi/pthreads.c"
(gdb)

As noticed by Nick, this "double printing" is not documented neither 
tested. I propose to remove it, it does not involved any regression in 
the testsuite( done for linux native target).

-- 
Denis





[-- Attachment #2: thread-select.patch --]
[-- Type: text/plain, Size: 875 bytes --]

2007-03-19  Denis Pilat  <denis.pilat@st.com>

	* thread.c (do_captured_thread_select): print_stack_frame use
	LOC_AND_ADDRESS for mi output.

Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.51
diff -u -p -r1.51 thread.c
--- thread.c	28 Feb 2007 17:35:01 -0000	1.51
+++ thread.c	19 Mar 2007 13:27:53 -0000
@@ -700,7 +700,12 @@ do_captured_thread_select (struct ui_out
   ui_out_text (uiout, target_tid_to_str (inferior_ptid));
   ui_out_text (uiout, ")]");
 
-  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC);
+  /* For mi, we just print location.  */
+  if (ui_out_is_mi_like_p (uiout))
+    print_stack_frame (get_selected_frame (NULL), 1, LOC_AND_ADDRESS);
+  else
+    print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC);
+
   return GDB_RC_OK;
 }
 

Re: [RFC] -thread-select double print stack frame

[Nick Roberts]

 > 2007-03-19  Denis Pilat  <denis.pilat@st.com>
 > 
 > 	* thread.c (do_captured_thread_select): print_stack_frame use
 > 	LOC_AND_ADDRESS for mi output.
 > 
 > Index: thread.c
 > ===================================================================
 > RCS file: /cvs/src/src/gdb/thread.c,v
 > retrieving revision 1.51
 > diff -u -p -r1.51 thread.c
 > --- thread.c	28 Feb 2007 17:35:01 -0000	1.51
 > +++ thread.c	19 Mar 2007 13:27:53 -0000
 > @@ -700,7 +700,12 @@ do_captured_thread_select (struct ui_out
 >    ui_out_text (uiout, target_tid_to_str (inferior_ptid));
 >    ui_out_text (uiout, ")]");
 >  
 > -  print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC);
 > +  /* For mi, we just print location.  */
 > +  if (ui_out_is_mi_like_p (uiout))
 > +    print_stack_frame (get_selected_frame (NULL), 1, LOC_AND_ADDRESS);
 > +  else
 > +    print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC);
 > +
 >    return GDB_RC_OK;
 >  }
 >  

While this surely fixes the immediate problem, I'm not sure it's the best way.
For example, we could have:

args.print_what = ui_out_is_mi_like_p (uiout) ? LOC_AND_ADDRESS : print_what

in print_stack_frame, to centralise things and remove the need for the clause
in normal_stop.

Furthermore, I don't understand why file and line details are duplicated in MI,
but not CLI.  It has something to do with uiout->flags not being 0 in MI (from
looking at print_source_lines_base).  The frame printing code is either one big
mess, or I'm not seeing the underlying structure at the moment.  Hopefully it's
the latter.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFC] -thread-select double print stack frame

[Denis PILAT]

> args.print_what = ui_out_is_mi_like_p (uiout) ? LOC_AND_ADDRESS : print_what
>
> in print_stack_frame, to centralise things and remove the need for the clause
> in normal_stop.
>
>
>   
Attach is a patch that fits Nick's comments and fixes the "double print" 
bug.
Stack frame printing under MI is centralized into print_stack_frame 
which alway prints location and address.
We don't need anymore specific code in infrun.c, but also is the 
-thread-info command I've submitted last week.

Ok for commit ?
-- 
Denis

2007-03-26  Denis Pilat  <denis.pilat@st.com>

	* stack.c (print_stack_frame): alway use LOC_AND_ADDRESS in mi output.
	* infrun.c (normal_stop): remove mi specific frame printing treatment.

Index: stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.142
diff -u -p -r1.142 stack.c
--- stack.c     27 Feb 2007 19:46:04 -0000      1.142
+++ stack.c     26 Mar 2007 13:14:25 -0000
@@ -105,7 +105,8 @@ print_stack_frame (struct frame_info *fr

   args.frame = frame;
   args.print_level = print_level;
-  args.print_what = print_what;
+  /* For mi, alway print location and address.  */
+  args.print_what = ui_out_is_mi_like_p (uiout) ? LOC_AND_ADDRESS : print_what;
   args.print_args = 1;

   catch_errors (print_stack_frame_stub, &args, "", RETURN_MASK_ALL);
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.223
diff -u -p -r1.223 infrun.c
--- infrun.c    9 Mar 2007 16:20:42 -0000       1.223
+++ infrun.c    26 Mar 2007 13:14:39 -0000
@@ -3200,10 +3200,6 @@ Further execution is probably impossible
            default:
              internal_error (__FILE__, __LINE__, _("Unknown value."));
            }
-         /* For mi, have the same behavior every time we stop:
-            print everything but the source line. */
-         if (ui_out_is_mi_like_p (uiout))
-           source_flag = LOC_AND_ADDRESS;

          if (ui_out_is_mi_like_p (uiout))
            ui_out_field_int (uiout, "thread-id",

Re: [RFC] -thread-select double print stack frame

[Daniel Jacobowitz]

On Mon, Mar 26, 2007 at 03:19:31PM +0200, Denis PILAT wrote:
> 2007-03-26  Denis Pilat  <denis.pilat@st.com>

> 	* stack.c (print_stack_frame): alway use LOC_AND_ADDRESS in mi output.
> 	* infrun.c (normal_stop): remove mi specific frame printing treatment.

This is basically OK; I agree with Nick that it's the right solution.
It needs some formatting fixes, though.

2007-03-26  Denis Pilat  <denis.pilat@st.com>

	* stack.c (print_stack_frame): Always use LOC_AND_ADDRESS in MI output.
	* infrun.c (normal_stop): Remove MI specific frame printing treatment.

Thanks for the patch.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] -thread-select double print stack frame

[Nick Roberts]

Daniel Jacobowitz writes:
 > On Mon, Mar 26, 2007 at 03:19:31PM +0200, Denis PILAT wrote:
 > > 2007-03-26  Denis Pilat  <denis.pilat@st.com>
 > 
 > > 	* stack.c (print_stack_frame): alway use LOC_AND_ADDRESS in mi output.
 > > 	* infrun.c (normal_stop): remove mi specific frame printing treatment.
 > 
 > This is basically OK; I agree with Nick that it's the right solution.
 > It needs some formatting fixes, though.

But why do you think that while file and line details are currenly duplicated
in MI, they are not in CLI?  (I mean what is the underlying logic/cause, not
the just the code differences).

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFC] -thread-select double print stack frame

[Daniel Jacobowitz]

On Wed, Mar 28, 2007 at 09:39:38AM +1200, Nick Roberts wrote:
> Daniel Jacobowitz writes:
>  > On Mon, Mar 26, 2007 at 03:19:31PM +0200, Denis PILAT wrote:
>  > > 2007-03-26  Denis Pilat  <denis.pilat@st.com>
>  > 
>  > > 	* stack.c (print_stack_frame): alway use LOC_AND_ADDRESS in mi output.
>  > > 	* infrun.c (normal_stop): remove mi specific frame printing treatment.
>  > 
>  > This is basically OK; I agree with Nick that it's the right solution.
>  > It needs some formatting fixes, though.
> 
> But why do you think that while file and line details are currenly duplicated
> in MI, they are not in CLI?  (I mean what is the underlying logic/cause, not
> the just the code differences).

Here's what I get from the CLI:

[Switching to thread 1 (Thread 46912506009296 (LWP 28747))]#0  main
(argc=1, argv=0x7fffffffe178)
    at /space/fsf/commit/src/gdb/gdb.c:28
28      {

The first line is the frame description and becomes the new-thread-id
and the frame={} tuple.  The second line ("28 {") comes from
print_source_lines (from lack of ui_source_list).  I think we need
both, because we use print_source_lines in other contexts too, but I'm
not sure about that.

So the short answer is that they are duplicated in the CLI too.  The
two copies are just formatted so differently that it isn't obvious.

This does make me wonder about the patch though.  Denis, could you
hold off on committing it?  Which duplicate copy are you eliminating?
Maybe we should diff two testsuite runs to see what else changes.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] -thread-select double print stack frame

[Nick Roberts]

 > > But why do you think that while file and line details are currenly
 > > duplicated in MI, they are not in CLI?  (I mean what is the underlying
 > > logic/cause, not the just the code differences).
 > 
 > Here's what I get from the CLI:
 > 
 > [Switching to thread 1 (Thread 46912506009296 (LWP 28747))]#0  main
 > (argc=1, argv=0x7fffffffe178)
 >     at /space/fsf/commit/src/gdb/gdb.c:28
 > 28      {
 > 
 > The first line is the frame description and becomes the new-thread-id
 > and the frame={} tuple.  The second line ("28 {") comes from
 > print_source_lines (from lack of ui_source_list).  I think we need
 > both, because we use print_source_lines in other contexts too, but I'm
 > not sure about that.
 > 
 > So the short answer is that they are duplicated in the CLI too.  The
 > two copies are just formatted so differently that it isn't obvious.

OK.  I think I understand the second call now.

I was looking specifically at this clause in print_source_lines_base:

  if (desc < 0)
    {
      last_source_error = desc;

      if (!noerror)
	{
	  char *name = alloca (strlen (s->filename) + 100);
	  sprintf (name, "%d\t%s", line, s->filename);
	  print_sys_errmsg (name, errno);
	}
      else
	ui_out_field_int (uiout, "line", line);
      ui_out_text (uiout, "\tin ");
      ui_out_field_string (uiout, "file", s->filename);
      ui_out_text (uiout, "\n");

      return;
    }

I think this code may only be reached when there is duplicated MI output.

 > This does make me wonder about the patch though.  Denis, could you
 > hold off on committing it?  Which duplicate copy are you eliminating?
 > Maybe we should diff two testsuite runs to see what else changes.

No, I think this change is alright (I haven't run the testsuite though.).

It's your decision but the next release is a long way off and if these changes
are not the right ones they are not far off.  So I would suggest committing
them all i.e this one, yours and Denis's -thread-* commands now.  That way
if there are problems we will quite likely discover them before the release.


-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFC] -thread-select double print stack frame

[Denis PILAT]

>
>  > This does make me wonder about the patch though.  Denis, could you
>  > hold off on committing it?  Which duplicate copy are you eliminating?
>  > Maybe we should diff two testsuite runs to see what else changes.
>
> No, I think this change is alright (I haven't run the testsuite though.).
>   
I do run the testsuite on i386-linux native target, no regression at all.
Can I commit or not ?
> It's your decision but the next release is a long way off and if these changes
> are not the right ones they are not far off.  So I would suggest committing
> them all i.e this one, yours and Denis's -thread-* commands now.  That way
> if there are problems we will quite likely discover them before the release.
>   
I will modify my patch on -thread-info new command once Daniel has 
committed.
I'll try to write a doc entry as well ...

-- 
Denis

Re: [RFC] -thread-select double print stack frame

[Daniel Jacobowitz]

On Wed, Mar 28, 2007 at 05:53:15PM +1200, Nick Roberts wrote:
> I was looking specifically at this clause in print_source_lines_base:
> 
>   if (desc < 0)

Right, that's where it comes from.

> I think this code may only be reached when there is duplicated MI output.

I don't know.  Some of the source-related commands may also call
print_source_lines, in which case they'll get here too - without a
frame printout.

>  > This does make me wonder about the patch though.  Denis, could you
>  > hold off on committing it?  Which duplicate copy are you eliminating?
>  > Maybe we should diff two testsuite runs to see what else changes.
> 
> No, I think this change is alright (I haven't run the testsuite though.).

You're right, anyway.  That's what I get for reviewing patches while
I'm so tired.  I was worried that the copy inside the frame={} tuple
was being eliminated by this change, but that's definitely not what
happens.  Only -thread-select of everything in our testsuite changed.

On Wed, Mar 28, 2007 at 10:25:37AM +0200, Denis PILAT wrote:
> I do run the testsuite on i386-linux native target, no regression at all.
> Can I commit or not ?

Yes, go ahead.  Thanks.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] -thread-select double print stack frame

[Denis PILAT]

>> I do run the testsuite on i386-linux native target, no regression at all.
>> Can I commit or not ?
>>     
>
> Yes, go ahead.  Thanks.
>
>   

Just committed !
Attached is the patch:
-- 
Denis

2007-03-29  Denis Pilat  <denis.pilat@st.com>

	* stack.c (print_stack_frame): Always use LOC_AND_ADDRESS in MI output.
	* infrun.c (normal_stop): Remove MI specific frame printing treatment.

Index: stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.142
diff -u -p -r1.142 stack.c
--- stack.c     27 Feb 2007 19:46:04 -0000      1.142
+++ stack.c     26 Mar 2007 13:14:25 -0000
@@ -105,7 +105,8 @@ print_stack_frame (struct frame_info *fr

  args.frame = frame;
  args.print_level = print_level;
-  args.print_what = print_what;
+  /* For mi, alway print location and address.  */
+  args.print_what = ui_out_is_mi_like_p (uiout) ? LOC_AND_ADDRESS : print_what;
  args.print_args = 1;

  catch_errors (print_stack_frame_stub, &args, "", RETURN_MASK_ALL);
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.223
diff -u -p -r1.223 infrun.c
--- infrun.c    9 Mar 2007 16:20:42 -0000       1.223
+++ infrun.c    26 Mar 2007 13:14:39 -0000
@@ -3200,10 +3200,6 @@ Further execution is probably impossible
           default:
             internal_error (__FILE__, __LINE__, _("Unknown value."));
           }
-         /* For mi, have the same behavior every time we stop:
-            print everything but the source line. */
-         if (ui_out_is_mi_like_p (uiout))
-           source_flag = LOC_AND_ADDRESS;

         if (ui_out_is_mi_like_p (uiout))
           ui_out_field_int (uiout, "thread-id",