PR threads/2015: Fix adjust_pc_after_break for thread debugging

PR threads/2015: Fix adjust_pc_after_break for thread debugging

[Andreas Schwab]

adjust_pc_after_break is doing the wrong thing during thread debugging
when the current thread is different from the thread when the debuggee was
stopped last.  The problem is that it calls currently_stepping, which
accesses global variables that are part of the thread context.  But the
context switch will only happen much later on.  The proposed fix will skip
the call when the current infrun context does not match the thread to be
examined.  This has been tested on x86_64-suse-linux and fixes 32
testcases without any regressions.

Andreas.

2005-09-28  Andreas Schwab  <schwab@suse.de>

	PR threads/2015
	* infrun.c (adjust_pc_after_break): Don't use the state from a
	non-matching thread context.

--- gdb/infrun.c	01 Aug 2005 11:52:13 +0200	1.203
+++ gdb/infrun.c	28 Sep 2005 00:03:47 +0200	
@@ -1200,8 +1200,12 @@ adjust_pc_after_break (struct execution_
       /* When using hardware single-step, a SIGTRAP is reported for
          both a completed single-step and a software breakpoint.  Need
          to differentiate between the two as the latter needs
-         adjusting but the former does not.  */
-      if (currently_stepping (ecs))
+         adjusting but the former does not.
+
+         When the thread to be examined does not match the current thread
+         context we can't use currently_stepping, so assume no
+         single-stepping in this case.  */
+      if (ptid_equal (ecs->ptid, inferior_ptid) && currently_stepping (ecs))
 	{
 	  if (prev_pc == breakpoint_pc
 	      && software_breakpoint_inserted_here_p (breakpoint_pc))

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: PR threads/2015: Fix adjust_pc_after_break for thread debugging

[Mark Kettenis]

> X-From_: gdb-patches-return-40942-m.m.kettenis=alumnus.utwente.nl@sources.redhat.com  Wed Sep 28 00:10:41 2005
> From: Andreas Schwab <schwab@suse.de>
> Date: Wed, 28 Sep 2005 00:10:18 +0200
> 
> adjust_pc_after_break is doing the wrong thing during thread debugging
> when the current thread is different from the thread when the debuggee was
> stopped last.  The problem is that it calls currently_stepping, which
> accesses global variables that are part of the thread context.  But the
> context switch will only happen much later on.  The proposed fix will skip
> the call when the current infrun context does not match the thread to be
> examined.  This has been tested on x86_64-suse-linux and fixes 32
> testcases without any regressions.

I'd really like to see this tested on some more systems though,
especially on systems where adjust_pc_after_break is a no-op and
systems with software single-step.

I can run a few tests later this week if you want.

Mark

Re: PR threads/2015: Fix adjust_pc_after_break for thread debugging

[Andreas Schwab]

Mark Kettenis <mark.kettenis@xs4all.nl> writes:

> I'd really like to see this tested on some more systems though,
> especially on systems where adjust_pc_after_break is a no-op and
> systems with software single-step.

When adjust_pc_after_break is a no-op then my patch has obviously no
effect at all.  The testcase works just fine on those systems (tested ia64
and ppc).

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: PR threads/2015: Fix adjust_pc_after_break for thread debugging

[Daniel Jacobowitz]

On Wed, Sep 28, 2005 at 12:10:18AM +0200, Andreas Schwab wrote:
> adjust_pc_after_break is doing the wrong thing during thread debugging
> when the current thread is different from the thread when the debuggee was
> stopped last.  The problem is that it calls currently_stepping, which
> accesses global variables that are part of the thread context.  But the
> context switch will only happen much later on.  The proposed fix will skip
> the call when the current infrun context does not match the thread to be
> examined.  This has been tested on x86_64-suse-linux and fixes 32
> testcases without any regressions.

Which test failures are these?  i.e. why doesn't anyone else see this
when they run the testsuite?

Offhand I'd be suspicious that this helped - some other thread probably
needs its PC adjusted and now may not be.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: PR threads/2015: Fix adjust_pc_after_break for thread debugging

[Daniel Jacobowitz]

On Tue, Sep 27, 2005 at 06:19:52PM -0400, Daniel Jacobowitz wrote:
> On Wed, Sep 28, 2005 at 12:10:18AM +0200, Andreas Schwab wrote:
> > adjust_pc_after_break is doing the wrong thing during thread debugging
> > when the current thread is different from the thread when the debuggee was
> > stopped last.  The problem is that it calls currently_stepping, which
> > accesses global variables that are part of the thread context.  But the
> > context switch will only happen much later on.  The proposed fix will skip
> > the call when the current infrun context does not match the thread to be
> > examined.  This has been tested on x86_64-suse-linux and fixes 32
> > testcases without any regressions.
> 
> Which test failures are these?  i.e. why doesn't anyone else see this
> when they run the testsuite?
> 
> Offhand I'd be suspicious that this helped - some other thread probably
> needs its PC adjusted and now may not be.

As a followup to my questions: it looks like the only globals which are
used are also in ecs, so currently_stepping could easily be fixed to
read only from ecs.  But it's not clear whether we'd want to do this
test on ecs, inferior_ptid, or both.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: PR threads/2015: Fix adjust_pc_after_break for thread debugging

[Andreas Schwab]

Daniel Jacobowitz <drow@false.org> writes:

> Which test failures are these?  i.e. why doesn't anyone else see this
> when they run the testsuite?

I've somehow lost the log files and had to rerun the testsuite, and now I
no longer see any difference in the test results.

> Offhand I'd be suspicious that this helped - some other thread probably
> needs its PC adjusted and now may not be.

The patch will never result in less adjustments, because the other arm of
the conditional will do the adjustment anyway if a breakpoint exists at
the current PC.  Note that prev_pc is also part of the thread context and
may be wrong for the current thread as well.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: PR threads/2015: Fix adjust_pc_after_break for thread debugging

[Daniel Jacobowitz]

On Wed, Sep 28, 2005 at 12:57:16AM +0200, Andreas Schwab wrote:
> Daniel Jacobowitz <drow@false.org> writes:
> 
> > Which test failures are these?  i.e. why doesn't anyone else see this
> > when they run the testsuite?
> 
> I've somehow lost the log files and had to rerun the testsuite, and now I
> no longer see any difference in the test results.
> 
> > Offhand I'd be suspicious that this helped - some other thread probably
> > needs its PC adjusted and now may not be.
> 
> The patch will never result in less adjustments, because the other arm of
> the conditional will do the adjustment anyway if a breakpoint exists at
> the current PC.  Note that prev_pc is also part of the thread context and
> may be wrong for the current thread as well.

Can you describe whatever testcase prompted you to make this change? 
Is there some way we can reproduce the situation?

I've been looking this morning at a (probably unrelated) bug which
would probably vanish if this patch were applied, but which was really
triggered by an uninitialized variable in infrun.  More to come once
I've tested.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: PR threads/2015: Fix adjust_pc_after_break for thread debugging

[Andreas Schwab]

Daniel Jacobowitz <drow@false.org> writes:

> Can you describe whatever testcase prompted you to make this change? 

It's attached to the PR.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: PR threads/2015: Fix adjust_pc_after_break for thread debugging

[Daniel Jacobowitz]

On Wed, Sep 28, 2005 at 05:47:19PM +0200, Andreas Schwab wrote:
> Daniel Jacobowitz <drow@false.org> writes:
> 
> > Can you describe whatever testcase prompted you to make this change? 
> 
> It's attached to the PR.

I'm sorry, my memory is going.  Definitely not the same bug then.  I
will take a further look this afternoon.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: PR threads/2015: Fix adjust_pc_after_break for thread debugging

[Daniel Jacobowitz]

On Wed, Sep 28, 2005 at 12:10:18AM +0200, Andreas Schwab wrote:
> adjust_pc_after_break is doing the wrong thing during thread debugging
> when the current thread is different from the thread when the debuggee was
> stopped last.  The problem is that it calls currently_stepping, which
> accesses global variables that are part of the thread context.  But the
> context switch will only happen much later on.  The proposed fix will skip
> the call when the current infrun context does not match the thread to be
> examined.  This has been tested on x86_64-suse-linux and fixes 32
> testcases without any regressions.

I've got no objection.  Mark asked you to test software single step and
non-decr-pc-after-break; I agree with you that the patch can't have any
effect if either of those is the case (see the context).  So I think
this is OK.  Mark?

I was worried about the case when the thread previously being stepped
is not the event thread.  But it seems to be OK and there are some
problems there already (we don't keep track of whether the single-step
succeeded or not in some cases, for instance).

-- 
Daniel Jacobowitz
CodeSourcery, LLC