Commit Message
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.
gdb/ChangeLog:
2014-04-21 Simon Marchi <simon.marchi@ericsson.com>
* infrun.c (resume): Remove should_resume (unused).
Comments
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);
> }
On 14-04-21 12:12 PM, Joel Brobecker wrote:
> 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?
I was afraid it would not be clear and I almost mentioned it in my
original message. There are a lot of +/- for a tiny change, because the
code looked like that:
int should_resume = 1;
/* Untouched stuff */
if (should_resume)
{
/* Lots of stuff that needs to be unindented. */
...
}
The part that was unindented by four spaces generates a lot of +/-, but
I didn't change anything else than that, I swear !
Simon
> >> 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?
>
> I was afraid it would not be clear and I almost mentioned it in my
> original message. There are a lot of +/- for a tiny change, because the
> code looked like that:
>
> int should_resume = 1;
>
> /* Untouched stuff */
>
> if (should_resume)
> {
> /* Lots of stuff that needs to be unindented. */
> ...
> }
>
> The part that was unindented by four spaces generates a lot of +/-, but
> I didn't change anything else than that, I swear !
Indeed. It went a little too fast - what threw me was the addition
of resume_ptid's declaration at the start of the function. I did
a quick "git diff -b" and it explains the patch.
The patch is approved, but we should probably mention the fact that
resume_ptid's declaration was moved up in the ChangeLog entry.
Here is a link to request an account on sourceware.org, if you do not
already have one (you can list me as your sponsor):
https://sourceware.org/cgi-bin/pdw/ps_form.cgi
Once that's out of the way, please commit an obvious patch to
the gdb/MAINTAINERS file adding yourself to the "COMMIT AFTER
APPROVAL" list (the patch should be treated like all other obvious
patches, ie a copy should be sent to this mailing-list). Once that's
all done, then you can push this commit as well. Please remember
to send a quick confirmation that the commit was pushed to this
mailing-list as well.
Thank you,
On 14-04-21 01:14 PM, Joel Brobecker wrote:
>>>> 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?
>>
>> I was afraid it would not be clear and I almost mentioned it in my
>> original message. There are a lot of +/- for a tiny change, because the
>> code looked like that:
>>
>> int should_resume = 1;
>>
>> /* Untouched stuff */
>>
>> if (should_resume)
>> {
>> /* Lots of stuff that needs to be unindented. */
>> ...
>> }
>>
>> The part that was unindented by four spaces generates a lot of +/-, but
>> I didn't change anything else than that, I swear !
>
> Indeed. It went a little too fast - what threw me was the addition
> of resume_ptid's declaration at the start of the function. I did
> a quick "git diff -b" and it explains the patch.
>
> The patch is approved, but we should probably mention the fact that
> resume_ptid's declaration was moved up in the ChangeLog entry.
Would this be a properly formatted CL entry ?
2014-04-21 Simon Marchi <simon.marchi@ericsson.com>
* infrun.c (resume): Remove should_resume (unused). Move up
declaration of resume_ptid.
> Here is a link to request an account on sourceware.org, if you do not
> already have one (you can list me as your sponsor):
> https://sourceware.org/cgi-bin/pdw/ps_form.cgi
Done.
> Once that's out of the way, please commit an obvious patch to
> the gdb/MAINTAINERS file adding yourself to the "COMMIT AFTER
> APPROVAL" list (the patch should be treated like all other obvious
> patches, ie a copy should be sent to this mailing-list). Once that's
> all done, then you can push this commit as well. Please remember
> to send a quick confirmation that the commit was pushed to this
> mailing-list as well.
>
> Thank you,
Ok.
Thanks,
Simon
> Would this be a properly formatted CL entry ?
>
> 2014-04-21 Simon Marchi <simon.marchi@ericsson.com>
>
> * infrun.c (resume): Remove should_resume (unused). Move up
> declaration of resume_ptid.
Almost! 2 spaces after periods. (sorry about all these nits, it's
a GNU Coding Standard requirement).
On 14-04-21 01:14 PM, Joel Brobecker wrote:
>>>> 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?
>>
>> I was afraid it would not be clear and I almost mentioned it in my
>> original message. There are a lot of +/- for a tiny change, because the
>> code looked like that:
>>
>> int should_resume = 1;
>>
>> /* Untouched stuff */
>>
>> if (should_resume)
>> {
>> /* Lots of stuff that needs to be unindented. */
>> ...
>> }
>>
>> The part that was unindented by four spaces generates a lot of +/-, but
>> I didn't change anything else than that, I swear !
>
> Indeed. It went a little too fast - what threw me was the addition
> of resume_ptid's declaration at the start of the function. I did
> a quick "git diff -b" and it explains the patch.
>
> The patch is approved, but we should probably mention the fact that
> resume_ptid's declaration was moved up in the ChangeLog entry.
>
> Here is a link to request an account on sourceware.org, if you do not
> already have one (you can list me as your sponsor):
> https://sourceware.org/cgi-bin/pdw/ps_form.cgi
>
> Once that's out of the way, please commit an obvious patch to
> the gdb/MAINTAINERS file adding yourself to the "COMMIT AFTER
> APPROVAL" list (the patch should be treated like all other obvious
> patches, ie a copy should be sent to this mailing-list). Once that's
> all done, then you can push this commit as well. Please remember
> to send a quick confirmation that the commit was pushed to this
> mailing-list as well.
>
> Thank you,
All right, I am pushing this!
@@ -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);
}