Re: [PATCH] Remove unused variable

[Joel Brobecker <brobecker@adacore.com>]

Simon,

> should_resume is set to 1 at the beginning and never changed.
> 
> The legal paperwork for people at Ericsson Montreal has been completed
> last week, so I would be ready to open an account to be able to submit
> patches.

Great!

> 
> gdb/ChangeLog:
> 
> 2014-04-21  Simon Marchi <simon.marchi@ericsson.com>
> 
> 	* infrun.c (resume): Remove should_resume (unused).

Unfortunately, your patch does much much much much much much much
more than just removing "should_resume" :-).

Can you please submit a patch that just removes that variable?

> 
> 
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 31bb132..49fd58c 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -1771,13 +1771,13 @@ user_visible_resume_ptid (int step)
>  void
>  resume (int step, enum gdb_signal sig)
>  {
> -  int should_resume = 1;
>    struct cleanup *old_cleanups = make_cleanup (resume_cleanups, 0);
>    struct regcache *regcache = get_current_regcache ();
>    struct gdbarch *gdbarch = get_regcache_arch (regcache);
>    struct thread_info *tp = inferior_thread ();
>    CORE_ADDR pc = regcache_read_pc (regcache);
>    struct address_space *aspace = get_regcache_aspace (regcache);
> +  ptid_t resume_ptid;
> 
>    QUIT;
> 
> @@ -1921,87 +1921,82 @@ a command like `return' or `jump' to continue execution."));
>        insert_breakpoints ();
>      }
> 
> -  if (should_resume)
> +  /* If STEP is set, it's a request to use hardware stepping
> +     facilities.  But in that case, we should never
> +     use singlestep breakpoint.  */
> +  gdb_assert (!(singlestep_breakpoints_inserted_p && step));
> +
> +  /* Decide the set of threads to ask the target to resume.  Start
> +     by assuming everything will be resumed, than narrow the set
> +     by applying increasingly restricting conditions.  */
> +  resume_ptid = user_visible_resume_ptid (step);
> +
> +  /* Maybe resume a single thread after all.  */
> +  if ((step || singlestep_breakpoints_inserted_p)
> +      && tp->control.trap_expected)
> +    {
> +      /* We're allowing a thread to run past a breakpoint it has
> +	 hit, by single-stepping the thread with the breakpoint
> +	 removed.  In which case, we need to single-step only this
> +	 thread, and keep others stopped, as they can miss this
> +	 breakpoint if allowed to run.  */
> +      resume_ptid = inferior_ptid;
> +    }
> +
> +  if (gdbarch_cannot_step_breakpoint (gdbarch))
>      {
> -      ptid_t resume_ptid;
> +      /* Most targets can step a breakpoint instruction, thus
> +	 executing it normally.  But if this one cannot, just
> +	 continue and we will hit it anyway.  */
> +      if (step && breakpoint_inserted_here_p (aspace, pc))
> +	step = 0;
> +    }
> 
> -      /* If STEP is set, it's a request to use hardware stepping
> -	 facilities.  But in that case, we should never
> -	 use singlestep breakpoint.  */
> -      gdb_assert (!(singlestep_breakpoints_inserted_p && step));
> +  if (debug_displaced
> +      && use_displaced_stepping (gdbarch)
> +      && tp->control.trap_expected)
> +    {
> +      struct regcache *resume_regcache = get_thread_regcache (resume_ptid);
> +      struct gdbarch *resume_gdbarch = get_regcache_arch (resume_regcache);
> +      CORE_ADDR actual_pc = regcache_read_pc (resume_regcache);
> +      gdb_byte buf[4];
> 
> -      /* Decide the set of threads to ask the target to resume.  Start
> -	 by assuming everything will be resumed, than narrow the set
> -	 by applying increasingly restricting conditions.  */
> -      resume_ptid = user_visible_resume_ptid (step);
> +      fprintf_unfiltered (gdb_stdlog, "displaced: run %s: ",
> +			  paddress (resume_gdbarch, actual_pc));
> +      read_memory (actual_pc, buf, sizeof (buf));
> +      displaced_step_dump_bytes (gdb_stdlog, buf, sizeof (buf));
> +    }
> 
> -      /* Maybe resume a single thread after all.  */
> -      if ((step || singlestep_breakpoints_inserted_p)
> -	  && tp->control.trap_expected)
> -	{
> -	  /* We're allowing a thread to run past a breakpoint it has
> -	     hit, by single-stepping the thread with the breakpoint
> -	     removed.  In which case, we need to single-step only this
> -	     thread, and keep others stopped, as they can miss this
> -	     breakpoint if allowed to run.  */
> -	  resume_ptid = inferior_ptid;
> -	}
> +  if (tp->control.may_range_step)
> +    {
> +      /* If we're resuming a thread with the PC out of the step
> +	 range, then we're doing some nested/finer run control
> +	 operation, like stepping the thread out of the dynamic
> +	 linker or the displaced stepping scratch pad.  We
> +	 shouldn't have allowed a range step then.  */
> +      gdb_assert (pc_in_thread_step_range (pc, tp));
> +    }
> 
> -      if (gdbarch_cannot_step_breakpoint (gdbarch))
> -	{
> -	  /* Most targets can step a breakpoint instruction, thus
> -	     executing it normally.  But if this one cannot, just
> -	     continue and we will hit it anyway.  */
> -	  if (step && breakpoint_inserted_here_p (aspace, pc))
> -	    step = 0;
> -	}
> +  /* Install inferior's terminal modes.  */
> +  target_terminal_inferior ();
> 
> -      if (debug_displaced
> -          && use_displaced_stepping (gdbarch)
> -          && tp->control.trap_expected)
> -        {
> -	  struct regcache *resume_regcache = get_thread_regcache (resume_ptid);
> -	  struct gdbarch *resume_gdbarch = get_regcache_arch (resume_regcache);
> -          CORE_ADDR actual_pc = regcache_read_pc (resume_regcache);
> -          gdb_byte buf[4];
> -
> -          fprintf_unfiltered (gdb_stdlog, "displaced: run %s: ",
> -                              paddress (resume_gdbarch, actual_pc));
> -          read_memory (actual_pc, buf, sizeof (buf));
> -          displaced_step_dump_bytes (gdb_stdlog, buf, sizeof (buf));
> -        }
> -
> -      if (tp->control.may_range_step)
> -	{
> -	  /* If we're resuming a thread with the PC out of the step
> -	     range, then we're doing some nested/finer run control
> -	     operation, like stepping the thread out of the dynamic
> -	     linker or the displaced stepping scratch pad.  We
> -	     shouldn't have allowed a range step then.  */
> -	  gdb_assert (pc_in_thread_step_range (pc, tp));
> -	}
> +  /* Avoid confusing the next resume, if the next stop/resume
> +     happens to apply to another thread.  */
> +  tp->suspend.stop_signal = GDB_SIGNAL_0;
> 
> -      /* Install inferior's terminal modes.  */
> -      target_terminal_inferior ();
> -
> -      /* Avoid confusing the next resume, if the next stop/resume
> -	 happens to apply to another thread.  */
> -      tp->suspend.stop_signal = GDB_SIGNAL_0;
> -
> -      /* Advise target which signals may be handled silently.  If we have
> -	 removed breakpoints because we are stepping over one (which can
> -	 happen only if we are not using displaced stepping), we need to
> -	 receive all signals to avoid accidentally skipping a breakpoint
> -	 during execution of a signal handler.  */
> -      if ((step || singlestep_breakpoints_inserted_p)
> -	  && tp->control.trap_expected
> -	  && !use_displaced_stepping (gdbarch))
> -	target_pass_signals (0, NULL);
> -      else
> -	target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass);
> +  /* Advise target which signals may be handled silently.  If we have
> +     removed breakpoints because we are stepping over one (which can
> +     happen only if we are not using displaced stepping), we need to
> +     receive all signals to avoid accidentally skipping a breakpoint
> +     during execution of a signal handler.  */
> +  if ((step || singlestep_breakpoints_inserted_p)
> +      && tp->control.trap_expected
> +      && !use_displaced_stepping (gdbarch))
> +    target_pass_signals (0, NULL);
> +  else
> +    target_pass_signals ((int) GDB_SIGNAL_LAST, signal_pass);
> 
> -      target_resume (resume_ptid, step, sig);
> -    }
> +  target_resume (resume_ptid, step, sig);
> 
>    discard_cleanups (old_cleanups);
>  }

-- 
Joel