[RFA] clean up temp sals in handle_inferior_event

[RFA] clean up temp sals in handle_inferior_event

[Michael Snyder]

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

Almost obvious, but I'll play it conservative.  ;-)

There were seven, and now eight, places in handle_inferior_event
where a local struct symtab_and_line is declared and used briefly.
One of them even opens and closes a block just for this purpose.

This patch merges those all into a single local temp variable.

They all initialize it, and all but one of them returns after
using it, so they can't interact.  Plus I ran the testsuites
with no regressions.

OK to check in?


[-- Attachment #2: tmp-sal.txt --]
[-- Type: text/plain, Size: 6943 bytes --]

2009-07-19  Michael Snyder  <msnyder@vmware.com>

	* infrun.c (handle_inferior_event): Clean up local tmp variables.

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.401
diff -u -p -r1.401 infrun.c
--- infrun.c	18 Jul 2009 23:35:31 -0000	1.401
+++ infrun.c	20 Jul 2009 02:21:49 -0000
@@ -2383,7 +2383,7 @@ handle_inferior_event (struct execution_
   int sw_single_step_trap_p = 0;
   int stopped_by_watchpoint;
   int stepped_after_stopped_by_watchpoint = 0;
-  struct symtab_and_line stop_pc_sal;
+  struct symtab_and_line stop_pc_sal, tmp_sal;
   enum stop_kind stop_soon;
 
   if (ecs->ws.kind != TARGET_WAITKIND_EXITED
@@ -3738,12 +3738,11 @@ infrun: not switching back to stepped th
 	{
 	  /* Set up a step-resume breakpoint at the address
 	     indicated by SKIP_SOLIB_RESOLVER.  */
-	  struct symtab_and_line sr_sal;
-	  init_sal (&sr_sal);
-	  sr_sal.pc = pc_after_resolver;
+	  init_sal (&tmp_sal);
+	  tmp_sal.pc = pc_after_resolver;
 
 	  insert_step_resume_breakpoint_at_sal (gdbarch,
-						sr_sal, null_frame_id);
+						tmp_sal, null_frame_id);
 	}
 
       keep_going (ecs);
@@ -3834,13 +3833,11 @@ infrun: not switching back to stepped th
 
 	  if (execution_direction == EXEC_REVERSE)
 	    {
-	      struct symtab_and_line sr_sal;
-
 	      /* Normal function call return (static or dynamic).  */
-	      init_sal (&sr_sal);
-	      sr_sal.pc = ecs->stop_func_start;
-		  insert_step_resume_breakpoint_at_sal (gdbarch,
-							sr_sal, null_frame_id);
+	      init_sal (&tmp_sal);
+	      tmp_sal.pc = ecs->stop_func_start;
+	      insert_step_resume_breakpoint_at_sal (gdbarch,
+						    tmp_sal, null_frame_id);
 	    }
 	  else
 	    insert_step_resume_breakpoint_at_caller (frame);
@@ -3862,12 +3859,11 @@ infrun: not switching back to stepped th
 
       if (real_stop_pc != 0 && in_solib_dynsym_resolve_code (real_stop_pc))
 	{
-	  struct symtab_and_line sr_sal;
-	  init_sal (&sr_sal);
-	  sr_sal.pc = ecs->stop_func_start;
+	  init_sal (&tmp_sal);
+	  tmp_sal.pc = ecs->stop_func_start;
 
 	  insert_step_resume_breakpoint_at_sal (gdbarch,
-						sr_sal, null_frame_id);
+						tmp_sal, null_frame_id);
 	  keep_going (ecs);
 	  return;
 	}
@@ -3878,19 +3874,15 @@ infrun: not switching back to stepped th
          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)
-	  {
-	    if (execution_direction == EXEC_REVERSE)
-	      handle_step_into_function_backward (gdbarch, ecs);
-	    else
-	      handle_step_into_function (gdbarch, ecs);
-	    return;
-	  }
-      }
+      tmp_sal = find_pc_line (ecs->stop_func_start, 0);
+      if (tmp_sal.line != 0)
+	{
+	  if (execution_direction == EXEC_REVERSE)
+	    handle_step_into_function_backward (gdbarch, ecs);
+	  else
+	    handle_step_into_function (gdbarch, 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
@@ -3908,11 +3900,10 @@ infrun: not switching back to stepped th
 	{
 	  /* Set a breakpoint at callee's start address.
 	     From there we can step once and be back in the caller.  */
-	  struct symtab_and_line sr_sal;
-	  init_sal (&sr_sal);
-	  sr_sal.pc = ecs->stop_func_start;
+	  init_sal (&tmp_sal);
+	  tmp_sal.pc = ecs->stop_func_start;
 	  insert_step_resume_breakpoint_at_sal (gdbarch,
-						sr_sal, null_frame_id);
+						tmp_sal, null_frame_id);
 	}
       else
 	/* Set a breakpoint at callee's return address (the address
@@ -3945,11 +3936,10 @@ infrun: not switching back to stepped th
 	  /* Stepped backward into the solib dynsym resolver.
 	     Set a breakpoint at its start and continue, then
 	     one more step will take us out.  */
-	  struct symtab_and_line sr_sal;
-	  init_sal (&sr_sal);
-	  sr_sal.pc = ecs->stop_func_start;
+	  init_sal (&tmp_sal);
+	  tmp_sal.pc = ecs->stop_func_start;
 	  insert_step_resume_breakpoint_at_sal (gdbarch, 
-						sr_sal, null_frame_id);
+						tmp_sal, null_frame_id);
 	  keep_going (ecs);
 	  return;
 	}
@@ -3971,17 +3961,15 @@ infrun: not switching back to stepped th
       if (real_stop_pc)
 	{
 	  /* And put the step-breakpoint there and go until there. */
-	  struct symtab_and_line sr_sal;
-
-	  init_sal (&sr_sal);	/* initialize to zeroes */
-	  sr_sal.pc = real_stop_pc;
-	  sr_sal.section = find_pc_overlay (sr_sal.pc);
+	  init_sal (&tmp_sal);	/* initialize to zeroes */
+	  tmp_sal.pc = real_stop_pc;
+	  tmp_sal.section = find_pc_overlay (tmp_sal.pc);
 
 	  /* Do not specify what the fp should be when we stop since
 	     on some machines the prologue is where the new fp value
 	     is established.  */
 	  insert_step_resume_breakpoint_at_sal (gdbarch,
-						sr_sal, null_frame_id);
+						tmp_sal, null_frame_id);
 
 	  /* Restart without fiddling with the step ranges or
 	     other state.  */
@@ -4065,13 +4053,11 @@ infrun: not switching back to stepped th
 		   ecs->event_thread->step_frame_id)
       && inline_skipped_frames (ecs->ptid))
     {
-      struct symtab_and_line call_sal;
-
       if (debug_infrun)
 	fprintf_unfiltered (gdb_stdlog,
 			    "infrun: stepped into inlined function\n");
 
-      find_frame_sal (get_current_frame (), &call_sal);
+      find_frame_sal (get_current_frame (), &tmp_sal);
 
       if (ecs->event_thread->step_over_calls != STEP_OVER_ALL)
 	{
@@ -4080,22 +4066,21 @@ infrun: not switching back to stepped th
 	     we were previously stepping, go down into the function
 	     first.  Otherwise stop at the call site.  */
 
-	  if (call_sal.line == ecs->event_thread->current_line
-	      && call_sal.symtab == ecs->event_thread->current_symtab)
+	  if (tmp_sal.line == ecs->event_thread->current_line
+	      && tmp_sal.symtab == ecs->event_thread->current_symtab)
 	    step_into_inline_frame (ecs->ptid);
 
 	  ecs->event_thread->stop_step = 1;
 	  print_stop_reason (END_STEPPING_RANGE, 0);
 	  stop_stepping (ecs);
-	  return;
 	}
       else
 	{
 	  /* For "next", we should stop at the call site if it is on a
 	     different source line.  Otherwise continue through the
 	     inlined function.  */
-	  if (call_sal.line == ecs->event_thread->current_line
-	      && call_sal.symtab == ecs->event_thread->current_symtab)
+	  if (tmp_sal.line == ecs->event_thread->current_line
+	      && tmp_sal.symtab == ecs->event_thread->current_symtab)
 	    keep_going (ecs);
 	  else
 	    {
@@ -4103,8 +4088,8 @@ infrun: not switching back to stepped th
 	      print_stop_reason (END_STEPPING_RANGE, 0);
 	      stop_stepping (ecs);
 	    }
-	  return;
 	}
+      return;
     }
 
   /* Look for "calls" to inlined functions, part two.  If we are still

Re: [RFA] clean up temp sals in handle_inferior_event

[Tom Tromey]

>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> writes:

Michael> There were seven, and now eight, places in handle_inferior_event
Michael> where a local struct symtab_and_line is declared and used briefly.
Michael> One of them even opens and closes a block just for this purpose.

Michael> This patch merges those all into a single local temp variable.

Michael> They all initialize it, and all but one of them returns after
Michael> using it, so they can't interact.  Plus I ran the testsuites
Michael> with no regressions.

I find the old code clearer.  In the old code, these temporary variables
are mostly declared in small blocks surrounding their point of use.
This means that it is very easy to reason about the variable: its entire
lifetime fits onto the screen at once.

After the patch, this is not so.  And, given that handle_inferior_event
is extremely long, I think this makes it trickier to understand.

I don't really do much work in this code, though, so perhaps I'm
speaking out of turn.  I'd be interested to hear what Pedro has to say.

Also, if there is a benefit other than aesthetics to changing this, that
could change my opinion.

Tom

Re: [RFA] clean up temp sals in handle_inferior_event

[Stan Shebs]

Tom Tromey wrote:
>>>>>> "Michael" == Michael Snyder <msnyder@vmware.com> writes:
>>>>>>             
>
> Michael> There were seven, and now eight, places in handle_inferior_event
> Michael> where a local struct symtab_and_line is declared and used briefly.
> Michael> One of them even opens and closes a block just for this purpose.
>
> Michael> This patch merges those all into a single local temp variable.
>
> Michael> They all initialize it, and all but one of them returns after
> Michael> using it, so they can't interact.  Plus I ran the testsuites
> Michael> with no regressions.
>
> I find the old code clearer.  In the old code, these temporary variables
> are mostly declared in small blocks surrounding their point of use.
> This means that it is very easy to reason about the variable: its entire
> lifetime fits onto the screen at once.
>   
I'd find it easier debuggingwise to have fewer of the small scopes, 
*but* to give the sals different names indicating their roles.  For me 
the confusion comes from using a single variable name in a function to 
mean several different things, whether or not each is in a different scope.

Stan