[RFA/patch] handle_inferior_event() extract some code into a separate function

[RFA/patch] handle_inferior_event() extract some code into a separate function

[Joel Brobecker]

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

[I think this patch was sort of pre-approved by Andrew, which explains
why I put "patch" in the subject, but I prefered to make sure and ask
for approval again. Hence the RFA]

Hello,

This change is related to a discussion started in the following thread:
http://sources.redhat.com/ml/gdb-patches/2003-12/msg00037.html

In particular, this implements the suggestion made by Andrew in the
following message:
http://sources.redhat.com/ml/gdb-patches/2003-12/msg00279.html

This patch implements the first part, which is to extract the code
inside the we-stepped-into-a-function if block, and move it to its
own function. It's fairly mechanical.

The next step will be to change the if condition itself to separate
out the targets where the new frame code is used from the ones where
the old one is still in use. I will send a patch as soon as this one
is reviewed.

2003-12-19  J. Brobecker  <brobecker@gnat.com>

        * infrun.c (handle_step_into_function): New function.
        (handle_inferior_event): Extract out some code into the new
        function above.

Tested on x86-linux, no regressions.

Ok to commit?

Thanks,
-- 
Joel

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

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.122
diff -u -p -r1.122 infrun.c
--- infrun.c	25 Nov 2003 16:01:36 -0000	1.122
+++ infrun.c	19 Dec 2003 14:33:05 -0000
@@ -987,6 +987,8 @@ struct execution_control_state
 
 void init_execution_control_state (struct execution_control_state *ecs);
 
+static void handle_step_into_function (struct execution_control_state *ecs,
+                                       CORE_ADDR real_stop_pc);
 void handle_inferior_event (struct execution_control_state *ecs);
 
 static void check_sigtramp2 (struct execution_control_state *ecs);
@@ -1236,6 +1238,94 @@ pc_in_sigtramp (CORE_ADDR pc)
   return PC_IN_SIGTRAMP (pc, name);
 }
 
+/* Handle the inferior event in the cases when we just stepped
+   into a function.  */
+
+static void
+handle_step_into_function (struct execution_control_state *ecs,
+                           CORE_ADDR real_stop_pc)
+{
+  if ((step_over_calls == STEP_OVER_NONE)
+      || ((step_range_end == 1)
+          && in_prologue (prev_pc, ecs->stop_func_start)))
+    {
+      /* I presume that step_over_calls is only 0 when we're
+         supposed to be stepping at the assembly language level
+         ("stepi").  Just stop.  */
+      /* Also, maybe we just did a "nexti" inside a prolog,
+         so we thought it was a subroutine call but it was not.
+         Stop as well.  FENN */
+      stop_step = 1;
+      print_stop_reason (END_STEPPING_RANGE, 0);
+      stop_stepping (ecs);
+      return;
+    }
+
+  if (step_over_calls == STEP_OVER_ALL || IGNORE_HELPER_CALL (stop_pc))
+    {
+      /* We're doing a "next".  */
+
+      if (pc_in_sigtramp (stop_pc)
+          && frame_id_inner (step_frame_id,
+                             frame_id_build (read_sp (), 0)))
+        /* We stepped out of a signal handler, and into its
+           calling trampoline.  This is misdetected as a
+           subroutine call, but stepping over the signal
+           trampoline isn't such a bad idea.  In order to do that,
+           we have to ignore the value in step_frame_id, since
+           that doesn't represent the frame that'll reach when we
+           return from the signal trampoline.  Otherwise we'll
+           probably continue to the end of the program.  */
+        step_frame_id = null_frame_id;
+
+      step_over_function (ecs);
+      keep_going (ecs);
+      return;
+    }
+
+  /* If we are in a function call trampoline (a stub between
+     the calling routine and the real function), locate the real
+     function.  That's what tells us (a) whether we want to step
+     into it at all, and (b) what prologue we want to run to
+     the end of, if we do step into it.  */
+  real_stop_pc = skip_language_trampoline (stop_pc);
+  if (real_stop_pc == 0)
+    real_stop_pc = SKIP_TRAMPOLINE_CODE (stop_pc);
+  if (real_stop_pc != 0)
+    ecs->stop_func_start = real_stop_pc;
+
+  /* If we have line number information for the function we
+     are thinking of stepping into, step into it.
+
+     If there are several symtabs at that PC (e.g. with include
+     files), just want to know whether *any* of them have line
+     numbers.  find_pc_line handles this.  */
+  {
+    struct symtab_and_line tmp_sal;
+
+    tmp_sal = find_pc_line (ecs->stop_func_start, 0);
+    if (tmp_sal.line != 0)
+      {
+        step_into_function (ecs);
+        return;
+      }
+  }
+
+  /* If we have no line number and the step-stop-if-no-debug
+     is set, we stop the step so that the user has a chance to
+     switch in assembly mode.  */
+  if (step_over_calls == STEP_OVER_UNDEBUGGABLE && step_stop_if_no_debug)
+    {
+      stop_step = 1;
+      print_stop_reason (END_STEPPING_RANGE, 0);
+      stop_stepping (ecs);
+      return;
+    }
+
+  step_over_function (ecs);
+  keep_going (ecs);
+  return;
+}
 
 /* Given an execution control state that has been freshly filled in
    by an event from the inferior, figure out what it means and take
@@ -2479,88 +2569,8 @@ process_event_stop_test:
       || ecs->stop_func_name == 0)
     {
       /* It's a subroutine call.  */
-
-      if ((step_over_calls == STEP_OVER_NONE)
-	  || ((step_range_end == 1)
-	      && in_prologue (prev_pc, ecs->stop_func_start)))
-	{
-	  /* I presume that step_over_calls is only 0 when we're
-	     supposed to be stepping at the assembly language level
-	     ("stepi").  Just stop.  */
-	  /* Also, maybe we just did a "nexti" inside a prolog,
-	     so we thought it was a subroutine call but it was not.
-	     Stop as well.  FENN */
-	  stop_step = 1;
-	  print_stop_reason (END_STEPPING_RANGE, 0);
-	  stop_stepping (ecs);
-	  return;
-	}
-
-      if (step_over_calls == STEP_OVER_ALL || IGNORE_HELPER_CALL (stop_pc))
-	{
-	  /* We're doing a "next".  */
-
-	  if (pc_in_sigtramp (stop_pc)
-	      && frame_id_inner (step_frame_id,
-				 frame_id_build (read_sp (), 0)))
-	    /* We stepped out of a signal handler, and into its
-	       calling trampoline.  This is misdetected as a
-	       subroutine call, but stepping over the signal
-	       trampoline isn't such a bad idea.  In order to do that,
-	       we have to ignore the value in step_frame_id, since
-	       that doesn't represent the frame that'll reach when we
-	       return from the signal trampoline.  Otherwise we'll
-	       probably continue to the end of the program.  */
-	    step_frame_id = null_frame_id;
-
-	  step_over_function (ecs);
-	  keep_going (ecs);
-	  return;
-	}
-
-      /* If we are in a function call trampoline (a stub between
-         the calling routine and the real function), locate the real
-         function.  That's what tells us (a) whether we want to step
-         into it at all, and (b) what prologue we want to run to
-         the end of, if we do step into it.  */
-      real_stop_pc = skip_language_trampoline (stop_pc);
-      if (real_stop_pc == 0)
-	real_stop_pc = SKIP_TRAMPOLINE_CODE (stop_pc);
-      if (real_stop_pc != 0)
-	ecs->stop_func_start = real_stop_pc;
-
-      /* If we have line number information for the function we
-         are thinking of stepping into, step into it.
-
-         If there are several symtabs at that PC (e.g. with include
-         files), just want to know whether *any* of them have line
-         numbers.  find_pc_line handles this.  */
-      {
-	struct symtab_and_line tmp_sal;
-
-	tmp_sal = find_pc_line (ecs->stop_func_start, 0);
-	if (tmp_sal.line != 0)
-	  {
-	    step_into_function (ecs);
-	    return;
-	  }
-      }
-
-      /* If we have no line number and the step-stop-if-no-debug
-         is set, we stop the step so that the user has a chance to
-         switch in assembly mode.  */
-      if (step_over_calls == STEP_OVER_UNDEBUGGABLE && step_stop_if_no_debug)
-	{
-	  stop_step = 1;
-	  print_stop_reason (END_STEPPING_RANGE, 0);
-	  stop_stepping (ecs);
-	  return;
-	}
-
-      step_over_function (ecs);
-      keep_going (ecs);
+      handle_step_into_function (ecs, real_stop_pc);
       return;
-
     }
 
   /* We've wandered out of the step range.  */

Re: [RFA/patch] handle_inferior_event() extract some code into a separate function

[Andrew Cagney]

> [I think this patch was sort of pre-approved by Andrew, which explains
> why I put "patch" in the subject, but I prefered to make sure and ask
> for approval again. Hence the RFA]
> 
> Hello,
> 
> This change is related to a discussion started in the following thread:
> http://sources.redhat.com/ml/gdb-patches/2003-12/msg00037.html
> 
> In particular, this implements the suggestion made by Andrew in the
> following message:
> http://sources.redhat.com/ml/gdb-patches/2003-12/msg00279.html
> 
> This patch implements the first part, which is to extract the code
> inside the we-stepped-into-a-function if block, and move it to its
> own function. It's fairly mechanical.
> 
> The next step will be to change the if condition itself to separate
> out the targets where the new frame code is used from the ones where
> the old one is still in use. I will send a patch as soon as this one
> is reviewed.

M'kay.

Andrew

Re: [RFA/patch] handle_inferior_event() extract some code into a separate function

[Joel Brobecker]

> >This patch implements the first part, which is to extract the code
> >inside the we-stepped-into-a-function if block, and move it to its
> >own function. It's fairly mechanical.
> >
> >The next step will be to change the if condition itself to separate
> >out the targets where the new frame code is used from the ones where
> >the old one is still in use. I will send a patch as soon as this one
> >is reviewed.
> 
> M'kay.

Thanks. It's in. Now to the next step...

-- 
Joel

Re: [RFA/patch] handle_inferior_event() extract some code into a separate function

[Mark Kettenis]

   Date: Sat, 3 Jan 2004 12:52:03 +0100
   From: Joel Brobecker <brobecker@gnat.com>

   > >This patch implements the first part, which is to extract the code
   > >inside the we-stepped-into-a-function if block, and move it to its
   > >own function. It's fairly mechanical.
   > >
   > >The next step will be to change the if condition itself to separate
   > >out the targets where the new frame code is used from the ones where
   > >the old one is still in use. I will send a patch as soon as this one
   > >is reviewed.
   > 
   > M'kay.

   Thanks. It's in. Now to the next step...

I get:

gcc -c -g -O2 ... -Werror /home/kettenis/sandbox/virgin-gdb/src/gdb/infrun.c
cc1: warnings being treated as errors
/home/kettenis/sandbox/virgin-gdb/src/gdb/infrun.c: In function `handle_inferior_event':
/home/kettenis/sandbox/virgin-gdb/src/gdb/infrun.c:1337: warning: `real_stop_pc' might be used uninitialized in this function

And as far as I can see, this is a genuine problem :-(.  Can you
please back that patch out?

Mark

Re: [RFA/patch] handle_inferior_event() extract some code into a separate function

[Joel Brobecker]

> I get:
> 
> gcc -c -g -O2 ... -Werror /home/kettenis/sandbox/virgin-gdb/src/gdb/infrun.c
> cc1: warnings being treated as errors
> /home/kettenis/sandbox/virgin-gdb/src/gdb/infrun.c: In function `handle_inferior_event':
> /home/kettenis/sandbox/virgin-gdb/src/gdb/infrun.c:1337: warning: `real_stop_pc' might be used uninitialized in this function
> 
> And as far as I can see, this is a genuine problem :-(.  Can you
> please back that patch out?

Backed out :(. Let me have a look.

-- 
Joel

[RFA] infrun.c:handle_inferior_event() tiny simplification (was "Re: [RFA/patch] handle_inferior_event() extract some code into a separate function")

[Joel Brobecker]

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

> gcc -c -g -O2 ... -Werror /home/kettenis/sandbox/virgin-gdb/src/gdb/infrun.c
> cc1: warnings being treated as errors
> /home/kettenis/sandbox/virgin-gdb/src/gdb/infrun.c: In function `handle_inferior_event':
> /home/kettenis/sandbox/virgin-gdb/src/gdb/infrun.c:1337: warning: `real_stop_pc' might be used uninitialized in this function

Thanks to Mark, I looked closer to this variable. It turns out that
this variable is very locally used in two blocks of the function,
vis:

void
handle_inferior_event (struct execution_control_state *ecs)
{
  [...]
  if ([test for subroutine call])\
    {
      [...]
      real_stop_pc = skip_language_trampoline (stop_pc);
      if (real_stop_pc == 0)
        real_stop_pc = SKIP_TRAMPOLINE_CODE (stop_pc);
      if (real_stop_pc != 0)
        ecs->stop_func_start = real_stop_pc;
      [...]
      return;
    }
  [...]
  if (IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, ecs->stop_func_name))
    {
      /* Determine where this trampoline returns.  */
      real_stop_pc = SKIP_TRAMPOLINE_CODE (stop_pc);

      /* Only proceed through if we know where it's going.  */
      if (real_stop_pc)
        {
          [...]
          sr_sal.pc = real_stop_pc;
          [...]
          return;
        }
    }
  [...]
}

So the real mistake I made was to make real_stop_pc a parameter
of the new function I introduced. It should have been a local
variable to that function.

Here is what I suggest:

  1. A patch to makes it more obvious that this variable is only
     locally used by defining it only inside these if blocks.
     Patch attached.

  2. Send an updated version of the patch I backed out where
     real_stop_pc is local variable to the new function, rather
     than a parameter (that was completely foolish since we don't
     even use the value that was passed and was not set in any case)
 
Here is the first patch:

2004-01-03  J. Brobecker  <brobecker@gnat.com>

        * infrun.c (handle_inferior_event): Move the declaration of
        real_stop_pc inside the if blocks where it is used.

OK to apply? Tested on x86-linux with GCC 3.2.3, no warning, and
no regression.

-- 
Joel

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

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.124
diff -u -p -r1.124 infrun.c
--- infrun.c	3 Jan 2004 13:02:00 -0000	1.124
+++ infrun.c	3 Jan 2004 14:47:01 -0000
@@ -1244,7 +1244,6 @@ pc_in_sigtramp (CORE_ADDR pc)
 void
 handle_inferior_event (struct execution_control_state *ecs)
 {
-  CORE_ADDR real_stop_pc;
   /* NOTE: cagney/2003-03-28: If you're looking at this code and
      thinking that the variable stepped_after_stopped_by_watchpoint
      isn't used, then you're wrong!  The macro STOPPED_BY_WATCHPOINT,
@@ -2479,6 +2478,7 @@ process_event_stop_test:
       || ecs->stop_func_name == 0)
     {
       /* It's a subroutine call.  */
+      CORE_ADDR real_stop_pc;
 
       if ((step_over_calls == STEP_OVER_NONE)
 	  || ((step_range_end == 1)
@@ -2582,7 +2582,7 @@ process_event_stop_test:
   if (IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, ecs->stop_func_name))
     {
       /* Determine where this trampoline returns.  */
-      real_stop_pc = SKIP_TRAMPOLINE_CODE (stop_pc);
+      CORE_ADDR real_stop_pc = SKIP_TRAMPOLINE_CODE (stop_pc);
 
       /* Only proceed through if we know where it's going.  */
       if (real_stop_pc)

Re: [RFA] infrun.c:handle_inferior_event() tiny simplification (was "Re: [RFA/patch] handle_inferior_event() extract some code into a separate function")

[Mark Kettenis]

   Date: Sat, 3 Jan 2004 16:01:07 +0100
   From: Joel Brobecker <brobecker@gnat.com>

   > infrun.c: In function `handle_inferior_event':
   > infrun.c:1337: warning: `real_stop_pc' might be used uninitialized in this function

   Thanks to Mark, I looked closer to this variable. It turns out that
   this variable is very locally used in two blocks of the function,
   vis:

   [...]

I agree with your analysis.

   So the real mistake I made was to make real_stop_pc a parameter
   of the new function I introduced. It should have been a local
   variable to that function.

   Here is what I suggest:

     1. A patch to makes it more obvious that this variable is only
	locally used by defining it only inside these if blocks.
	Patch attached.

     2. Send an updated version of the patch I backed out where
	real_stop_pc is local variable to the new function, rather
	than a parameter (that was completely foolish since we don't
	even use the value that was passed and was not set in any case)

   Here is the first patch:

   2004-01-03  J. Brobecker  <brobecker@gnat.com>

	   * infrun.c (handle_inferior_event): Move the declaration of
	   real_stop_pc inside the if blocks where it is used.

   OK to apply? Tested on x86-linux with GCC 3.2.3, no warning, and
   no regression.

Sounds great to me.  Consider the other patch pre-approved.

Mark

Re: [RFA] infrun.c:handle_inferior_event() tiny simplification (was "Re: [RFA/patch] handle_inferior_event() extract some code into a separate function")

[Joel Brobecker]

>    Here is what I suggest:
> 
>      1. A patch to makes it more obvious that this variable is only
> 	locally used by defining it only inside these if blocks.
> 	Patch attached.
> 
>      2. Send an updated version of the patch I backed out where
> 	real_stop_pc is local variable to the new function, rather
> 	than a parameter (that was completely foolish since we don't
> 	even use the value that was passed and was not set in any case)
> 
>    Here is the first patch:
> 
>    2004-01-03  J. Brobecker  <brobecker@gnat.com>
> 
> 	   * infrun.c (handle_inferior_event): Move the declaration of
> 	   real_stop_pc inside the if blocks where it is used.
> 
>    OK to apply? Tested on x86-linux with GCC 3.2.3, no warning, and
>    no regression.
> 
> Sounds great to me.  Consider the other patch pre-approved.

Thanks a lot. The first one has just been committed.
The second one is coming soon.

-- 
Joel