[RFC/RFA] continue stepping if landed in new range of same line

[RFC/RFA] continue stepping if landed in new range of same line

[Joel Brobecker]

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

Hello,

This is a minor issue that I noticed while working on something else.
I noticed this on x86-linux, using a GNAT compiler based on GCC 4.1
and it can be reproduced using today's HEAD debugger (I used gdb
6.7.50.20071217-cvs).

I have pasted the sources I used to reproduce the problem at the end
of this email. To build them, just do:

    % gnatmake -g p

Break at line 22, and then try to step over it:

    (gdb) break pck.adb:22
    Breakpoint 1 at 0x804a32d: file pck.adb, line 22.
    (gdb) run
    [...]
    Breakpoint 1, pck.lock.update (<O>=(system.address) 0x8074b20, 
        <P>=(system.address) 0xbfcadcc8, <E>=1) at pck.adb:22
    22            end Update;
    (gdb) n
    22            end Update;

As you can see, the "next" command stops in the same line.

As it turns out, there are two block of instructions associated
to line 22.  The assembly code looks like this:

   - Line 22: A jump to the other line 22
              Some other code
   - Line 19: [...]
   - Line 22: The function epilogue

After the jump, the inferior stops and GDB finds that we stepped
inside the last line of our function, and decides that we should
stop:

  if (ecs->stop_func_end && ecs->sal.end >= ecs->stop_func_end)
    {
      /* If this is the last line of the function, don't keep stepping
         (it would probably step us out of the function).
         This is particularly necessary for a one-line function,
         in which after skipping the prologue we better stop even though
         we will be in mid-line.  */

This code predates the public CVS so I wasn't able to find much
besides the comment. After reading this comment, I am no longer
sure of what to do.

The behavior that I observed is a bit surprising, and I am not sure that
the comment above meant to include a case like mine. However, can we
still maintain support for one-line functions, and yet handle the case
above? After thinking about it for a while, I think it's possible, but
it requires a bit a processing.

Originally, I just added some code that would continue stepping if we
entered a new range with the same line number (within the same function
call), but I'm now realizing that this is just defeating exactly what
the code above was trying to achieve.

So I removed that code I added, and enhance the check above to
verify that we are inside what looks like a one-line function before
stopping.  This fixes my testcase, and introduces no regression.

2007-12-19  Joel Brobecker  <brobecker@adacore.com>

	* infrun.c (handle_inferior_event): Stop in the last line of
        a function only if it is a one-line function.

Tested on x86-linux.  The code inside the new if statement is completely
unchanged, just indented because of the extra if level.

Thoughts?  Note that the behavior dysfunction is minor enough in my eyes
that I would be content to let it go...

Thanks!
-- 
Joel

package Pck is

   protected Lock is
      function Get return Integer;
      procedure Set;
      procedure Set (X : Integer);
      entry Update (Val : Integer);
   private
      Dummy : Integer := 0;
   end Lock;

end Pck;
package body Pck is

   protected body Lock is
      function Get return Integer is
      begin
         return Dummy;
      end Get;

      procedure Set is
      begin
         Dummy := 1;
      end Set;

      procedure Set (X : Integer) is
      begin
         Dummy := X;
      end Set;

      entry Update (Val : Integer) when True is
      begin
         Dummy := Val;
      end Update;  --  <--  LINE 22
   end Lock;

end Pck;
with Pck; use Pck;

procedure P is
   Value : Integer;
begin
   Lock.Set;
   Lock.Set (1);
   Value := Lock.Get;
   Lock.Update (2);
end P;


[-- Attachment #2: 02-infrun.c.diff --]
[-- Type: text/plain, Size: 1703 bytes --]

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.259
diff -u -p -r1.259 infrun.c
--- infrun.c	16 Dec 2007 19:14:23 -0000	1.259
+++ infrun.c	19 Dec 2007 07:57:28 -0000
@@ -2704,17 +2704,22 @@ process_event_stop_test:
 
   if (ecs->stop_func_end && ecs->sal.end >= ecs->stop_func_end)
     {
-      /* If this is the last line of the function, don't keep stepping
-         (it would probably step us out of the function).
-         This is particularly necessary for a one-line function,
-         in which after skipping the prologue we better stop even though
-         we will be in mid-line.  */
-      if (debug_infrun)
-	 fprintf_unfiltered (gdb_stdlog, "infrun: stepped to a different function\n");
-      stop_step = 1;
-      print_stop_reason (END_STEPPING_RANGE, 0);
-      stop_stepping (ecs);
-      return;
+      /* We landed in the last line of the function.  To help debugging
+         one-line functions, always stop (even if stopping mid-line) when
+         the function starts and finishes in the same line.  */
+      struct symtab_and_line fun_start_sal;
+      
+      fun_start_sal = find_pc_line (ecs->stop_func_start, 0);
+      if (fun_start_sal.line != 0 && fun_start_sal.line == ecs->sal.line)
+        {
+          if (debug_infrun)
+             fprintf_unfiltered (gdb_stdlog,
+                                 "infrun: stepped to a different function\n");
+          stop_step = 1;
+          print_stop_reason (END_STEPPING_RANGE, 0);
+          stop_stepping (ecs);
+          return;
+        }
     }
   step_range_start = ecs->sal.pc;
   step_range_end = ecs->sal.end;

Re: [RFC/RFA] continue stepping if landed in new range of same line

[Daniel Jacobowitz]

On Wed, Dec 19, 2007 at 11:59:03AM +0400, Joel Brobecker wrote:
> After the jump, the inferior stops and GDB finds that we stepped
> inside the last line of our function, and decides that we should
> stop:
> 
>   if (ecs->stop_func_end && ecs->sal.end >= ecs->stop_func_end)
>     {
>       /* If this is the last line of the function, don't keep stepping
>          (it would probably step us out of the function).
>          This is particularly necessary for a one-line function,
>          in which after skipping the prologue we better stop even though
>          we will be in mid-line.  */
> 
> This code predates the public CVS so I wasn't able to find much
> besides the comment. After reading this comment, I am no longer
> sure of what to do.
> 
> The behavior that I observed is a bit surprising, and I am not sure that
> the comment above meant to include a case like mine. However, can we
> still maintain support for one-line functions, and yet handle the case
> above? After thinking about it for a while, I think it's possible, but
> it requires a bit a processing.

I don't like your patch very much, since it assumes things about code
based on the structure of the source (and there are some pathological
cases, like functions which start in one file and end in another).
I'd rather figure out if the above quoted code is still necessary.  If
we were on the last line of the function and told to step, why
shouldn't we step out of it?


-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC/RFA] continue stepping if landed in new range of same line

[Joel Brobecker]

> I don't like your patch very much, since it assumes things about code
> based on the structure of the source (and there are some pathological
> cases, like functions which start in one file and end in another).

To be honest, I didn't like it very much either. I didn't know about
functions that start and end in different files! Is that through
inlining?

> I'd rather figure out if the above quoted code is still necessary.
> If we were on the last line of the function and told to step, why
> shouldn't we step out of it?

I am guessing that this is to handle the following situation:

    void foo (void) { bar (); baz (); }

When you break on foo, and then do a next:

    (gdb) break foo
    (gdb) cont
    [landing in foo, after function prologue]
    (gdb) next
    [executes "bar ();" statement, stops at call to baz]
    (gdb) next
    [executes "baz ();" statement, stops at epilogue]
    (gdb) next
    [step out of function]

Personally, I have always considered that "next" was supposed to
step until reaching a different line.  But a discussion that we had
internally at AdaCore a while back about something slightly different
made me realize that it's not that obvious. Just to explain a bit
more, we had some code like this:

    10. Foo (Parameter_1 => 2.0,
    11.      Parameter_2 => Call_Me (Argument => 1),
    12.      Parameter_3 => Do_Nothing (Ignore => 3));

Some of the engineers were arguing that if we were stopped at line 10,
then "next" should first get us to line 11 (call to Call_Me), and then
line 11 (call to Do_Nothing), and then back to line 10 for the actual
call to Foo. On the other hand, I was arguing that I expect the first
next to land past the call to Foo.

The debugger is actually not in complete control of what it does,
since it depends on how the compiler generates the line info.  But
this illustrates why what I no longer see the semantics of the "next"
command as obvious anymore.

Back to our case above, I personally would argue that the first
next should step all the way to out of the function. But whether
this would be the most useful behavior depends on the user (type
of code that he usually debugs, and how he likes to debug).
I hope that one-lines above are very rare, and perhaps we should
not worry about them.

-- 
Joel

Re: [RFC/RFA] continue stepping if landed in new range of same line

[Daniel Jacobowitz]

On Thu, Dec 20, 2007 at 09:26:17AM +0400, Joel Brobecker wrote:
> > I don't like your patch very much, since it assumes things about code
> > based on the structure of the source (and there are some pathological
> > cases, like functions which start in one file and end in another).
> 
> To be honest, I didn't like it very much either. I didn't know about
> functions that start and end in different files! Is that through
> inlining?

Much simpler than that.

#define FN foo
#include "start-fn.h"
  retval = arg1 + arg2;
#include "end-fn.h"

Finagle the comments just right and this will open on line 2 of
start-fn.h and close on line 2 of end-fn.h.

You might be able to arrange this with inlining or macros, too.

> > I'd rather figure out if the above quoted code is still necessary.
> > If we were on the last line of the function and told to step, why
> > shouldn't we step out of it?
> 
> I am guessing that this is to handle the following situation:
> 
>     void foo (void) { bar (); baz (); }

Amusingly, when you copy and paste this into Emacs, it winds up in
perfect GNU style on five lines...

I ran the experiment.  With the function on five lines, next goes from
bar() to baz() and then to }.  With the function on one line, it goes
all the way from bar() back to the caller.  So, maybe it was intended
to handle this case, but it doesn't.

My best guess is that it was design to handle a single-line function
without a call, to prevent us from skipping from the prologue all the
way out.  But I think other measures will prevent that too.

I think that code can probably change.  But you might want to try
debugging a couple of optimized cases to see if the behavior stays
sensible...

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC/RFA] continue stepping if landed in new range of same line

[Joel Brobecker]

> >     void foo (void) { bar (); baz (); }
> 
> Amusingly, when you copy and paste this into Emacs, it winds up in
> perfect GNU style on five lines...

I am wondering what emacs would do with the following example:

    void increment (int *a, int *b) { *a = *a + 1; *b = *b + 1; }

I wanted to try, but I must be missing something in my setup as
emacs doesn't indent either cases.

> I ran the experiment.  With the function on five lines, next goes from
> bar() to baz() and then to }.  With the function on one line, it goes
> all the way from bar() back to the caller.  So, maybe it was intended
> to handle this case, but it doesn't.

I think that the debugger would need the help of the compiler in order
to be able to do that. With the example above, I get the following code
on x86:

    increment:
            # f.c:1
            .file 1 "f.c"
            .loc 1 1 0
            pushl   %ebp    #
            movl    %esp, %ebp      #,

            # f.c:1
            .loc 1 1 0
            movl    8(%ebp), %eax   # a, a
            movl    12(%ebp), %edx  # b, b
            incl    (%eax)  #* a
            incl    (%edx)  #* b
            popl    %ebp    #
            ret

As you can see, the compiler repeats line 1 at the first instruction
past the prologue, but that's it. If the compiler had emitted
something a new line 1 at the epilogue, here is what the debugger
would do (does, in fact):

    (gdb) run
    Starting program: /home/no-backup/brobecke/next/C/g 
    
    Breakpoint 1, increment (a=0xbf9dd220, b=0xbf9dd21c) at f.c:1
    1       void increment (int *a, int *b) { *a = *a + 1; *b = *b + 1; }
    (gdb) n
    1       void increment (int *a, int *b) { *a = *a + 1; *b = *b + 1; }
    (gdb) x /i $pc
    0x80483cd <increment+13>:       pop    %ebp

On the other hand, explicitly separating the two statements with
an extra line as follow:

        # f.c:1
        .loc 1 1 0
        movl    8(%ebp), %eax   # a, a
        incl    (%eax)  #* a
        # f.c:1
        .loc 1 1 0
        movl    12(%ebp), %edx  # b, b
        incl    (%edx)  #* b
        # f.c:1
        .loc 1 1 0
        popl    %ebp    #
        ret

Does not allow us to stop before the second statement.

> My best guess is that it was design to handle a single-line function
> without a call, to prevent us from skipping from the prologue all the
> way out.  But I think other measures will prevent that too.

That's the part that I am no longer sure I understand. Which scenario
would that be? To me, after having stopped at the beginning of a
procedure, just past the prologue, and doing a next as above.
Right now, with the debugging info that is currently generated,
we do skip the function all the way out. However, if we're inside
the prologue: we do stop at the first instruction first. Maybe
that's what this code is trying to achieve.

Indeed, when I deactived the code that checks for the last line
in our function, here is the new behavior:

    (gdb) b *increment
    Breakpoint 1 at 0x80483c0: file f.c, line 1.
    (gdb) run
    Starting program: /home/no-backup/brobecke/next/C/g 
    
    Breakpoint 1, increment (a=0xbfd16d60, b=0xbfd16d5c) at f.c:1
    1       void increment (int *a, int *b) { *a = *a + 1; *b = *b + 1; }
    (gdb) n
    main () at g.c:12
    12        printf ("a = %d, b = %d\n", a, b);

Before I disabled this code, GDB would stop at line f.c:1 one more
time before landing back in the caller.

Perhaps if this is a requirement, we might want to add a testcase
for it in our testsuite.  Optimization is not necessary in order
to reproduce this...  Just for kicks, I ran the testsuite with
the disabled code, to see if anything would fail because of it,
and not unexpectedly, nothing did...

-- 
Joel

Re: [RFC/RFA] continue stepping if landed in new range of same line

[Daniel Jacobowitz]

On Fri, Dec 21, 2007 at 10:07:30AM +0400, Joel Brobecker wrote:
> I am wondering what emacs would do with the following example:
> 
>     void increment (int *a, int *b) { *a = *a + 1; *b = *b + 1; }
> 
> I wanted to try, but I must be missing something in my setup as
> emacs doesn't indent either cases.

You should just need to be in c-mode.  Might need
(c-toggle-auto-state 1) too (C-c C-a).

void increment (int *a, int *b)
{
  *a = *a + 1;
  *b = *b + 1;
}

(I was wrong about it being perfect; still, pretty good)
 
> I think that the debugger would need the help of the compiler in order
> to be able to do that. With the example above, I get the following code
> on x86:

Ideally we'd combine this with the is_stmt flag and column number
support, and put a caret where we wanted.

> Indeed, when I deactived the code that checks for the last line
> in our function, here is the new behavior:
> 
>     (gdb) b *increment
>     Breakpoint 1 at 0x80483c0: file f.c, line 1.
>     (gdb) run
>     Starting program: /home/no-backup/brobecke/next/C/g 
>     
>     Breakpoint 1, increment (a=0xbfd16d60, b=0xbfd16d5c) at f.c:1
>     1       void increment (int *a, int *b) { *a = *a + 1; *b = *b + 1; }
>     (gdb) n
>     main () at g.c:12
>     12        printf ("a = %d, b = %d\n", a, b);
> 
> Before I disabled this code, GDB would stop at line f.c:1 one more
> time before landing back in the caller.
> 
> Perhaps if this is a requirement, we might want to add a testcase
> for it in our testsuite.  Optimization is not necessary in order
> to reproduce this...  Just for kicks, I ran the testsuite with
> the disabled code, to see if anything would fail because of it,
> and not unexpectedly, nothing did...

To be honest, I find this behavior a bit surprising.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC/RFA] continue stepping if landed in new range of same line

[Joel Brobecker]

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

> Might need (c-toggle-auto-state 1) too (C-c C-a).

Indeed, that enabled the auto-indenting. Thanks! I used to be an emacs
aficionado but a couple of years no slow (and shared) Solaris machines
had me converted to vim ;-).

> To be honest, I find this behavior a bit surprising.

Me too. I don't really like to abritrarily change the behavior of
the debugger, which is why I was trying to work around it, but it looks
like from our experiments that the originally intended behavior might
no longer be possible because of the current debugging info. And it looks
like it has been so for a number of years already - I looked at the
output generated by GCC 3.2.3, and we have the same line table.

I wouldn't mind removing the code altogether; as a bonus it would also
reduce the size of handle_inferior_event. I can send a patch now, or
I can poll users on gdb@ and see what they think... Honestly, given
the likeliness of one-line functions, I'd just say it's not very
important to keep that special case.

2007-12-22  Joel Brobecker  <brobecker@adacore.com>

        * infrun.c (handle_inferior_event): Remove code that made us
        stop when stepping into the last line of the current function.

Tested on x86-linux, no regression.

-- 
Joel

[-- Attachment #2: hie.diff --]
[-- Type: text/plain, Size: 1168 bytes --]

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.260
diff -u -p -r1.260 infrun.c
--- infrun.c	19 Dec 2007 05:16:35 -0000	1.260
+++ infrun.c	22 Dec 2007 05:47:10 -0000
@@ -2702,20 +2702,6 @@ process_event_stop_test:
      new line in mid-statement, we continue stepping.  This makes
      things like for(;;) statements work better.)  */
 
-  if (ecs->stop_func_end && ecs->sal.end >= ecs->stop_func_end)
-    {
-      /* If this is the last line of the function, don't keep stepping
-         (it would probably step us out of the function).
-         This is particularly necessary for a one-line function,
-         in which after skipping the prologue we better stop even though
-         we will be in mid-line.  */
-      if (debug_infrun)
-	 fprintf_unfiltered (gdb_stdlog, "infrun: stepped to a different function\n");
-      stop_step = 1;
-      print_stop_reason (END_STEPPING_RANGE, 0);
-      stop_stepping (ecs);
-      return;
-    }
   step_range_start = ecs->sal.pc;
   step_range_end = ecs->sal.end;
   step_frame_id = get_frame_id (get_current_frame ());

Re: [RFC/RFA] continue stepping if landed in new range of same line

[Daniel Jacobowitz]

On Sat, Dec 22, 2007 at 10:01:37AM +0400, Joel Brobecker wrote:
> I wouldn't mind removing the code altogether; as a bonus it would also
> reduce the size of handle_inferior_event. I can send a patch now, or
> I can poll users on gdb@ and see what they think... Honestly, given
> the likeliness of one-line functions, I'd just say it's not very
> important to keep that special case.

There are a few one line functions in the testsuite.  In my opinion /
experience, they cover all the cases that really need to be covered;
if we find an omission, we can add some more tests :-)

> 2007-12-22  Joel Brobecker  <brobecker@adacore.com>
> 
>         * infrun.c (handle_inferior_event): Remove code that made us
>         stop when stepping into the last line of the current function.
> 
> Tested on x86-linux, no regression.

This is OK as far as I'm concerned.  And I am always in favor of
simplifying handle_inferior_event.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC/RFA] continue stepping if landed in new range of same line

[Joel Brobecker]

> > 2007-12-22  Joel Brobecker  <brobecker@adacore.com>
> > 
> >         * infrun.c (handle_inferior_event): Remove code that made us
> >         stop when stepping into the last line of the current function.
> > 
> > Tested on x86-linux, no regression.
> 
> This is OK as far as I'm concerned.  And I am always in favor of
> simplifying handle_inferior_event.

Great! I have checked this change in.
Thanks, Daniel.

-- 
Joel