This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: Patch for 'next' fails to set internal bp in non-stop
> -----Original Message-----
> From: Pedro Alves [mailto:pedro@codesourcery.com]
> Sent: Tuesday, April 19, 2011 7:44 AM
> To: gdb-patches@sourceware.org
> Cc: Marc Khouzam
> Subject: Re: Patch for 'next' fails to set internal bp in non-stop
>
> On Sunday 17 April 2011 14:12:29, Marc Khouzam wrote:
>
> > This is in relation to the issue I reported in
> > http://sourceware.org/ml/gdb/2011-04/msg00060.html
> > where doing a 'next' would fail to set its internal breakpoint
> > in non-stop mode.
> >
> > I debugged the issue further and I have a fix to propose below.
> > Here is what I think is happening.
> >
> > During the 'next' operation, GDB tries to set a bunch of breakpoints
> > including an internal one for the 'next' itself. This is done
> > in insert_breakpoint_locations() from breakpoint.c
> >
> > In GDB 7.0, where the problem does _not_ occur, when setting
> > all these breakpoints, GDB has its focus on the thread that
> is being
> > stepped, and that thread is of course not running so ptrace
> can access it.
> >
> > Starting with GDB 7.1, where multi-exec is supported, when GDB tries
> > to set breakpoints, it needs to change its focus to a
> thread of the right
> > program space for each breakpoint. When it is time to set
> the internal
> > breakpoint for the 'next' operation, GDB selects a thread using
> > any_live_thread_of_process() from thread.c. This method looks for
> > any thread that is in the state THREAD_STOPPED if
> available. The problem
> > is that because of the 'next' operation, the thread GDB is
> stepping has
> > already changed state to THREAD_RUNNING. If there are no
> other thread
> > STOPPED, then _any_ RUNNING thread is returned, and unless
> it happens
> > to be the thread that is being stepped, the ptrace
> operation will fail with
>
> Great investigation, many thanks.
It was fun. I got to use the Eclipse FE to debug some serious
C code! I was quite pleased with it :-)
> > (gdb) Warning:
> > Cannot insert breakpoint 0. <------ Notice #0 indicating
> internal breakpoint
> > Error accessing memory address 0x80484ef: Input/output error.
> >
> > What I propose is that GDB should try to return the thread
> that is being
> > stepped since it is not actually executing. I found that
> GDB has the
> > knowledge that a thread is RUNNING but not executing. The fix below
> > changes any_live_thread_of_process() to first prioritize a STOPPED
> > thread (like before), but then to prioritize a thread that
> has its executing_
> > flag set to false and only after that to fall back on an
> executing_ thread.
> >
> > This change seems pretty safe to me since it returns a
> thread that the
> > method could have returned before, it we were lucky; it is
> just that now,
> > we make sure it always does.
>
> I think we could always just pay attention to the executing_ flag,
> provided the thread is not known gone (THREAD_EXITED).
> We only really care if the thread is executing or not, in order
> to use it as context to poke at memory, not what the frontend
> believes the thread state is.
Nicer.
> > I find this problem to be pretty serious for people using non-stop
> > and if the fix is approved, I would like to put it in HEAD,
> 7.3 and 7.2
> > (is 7.2.1 still going to be released?)
>
> I agree. (I don't know; IMO, it should).
>
> > 2011-04-17 Marc Khouzam <marc.khouzam@ericsson.com>
> >
> > * thread.c (any_live_thread_of_process): Prioritize a thread
> > that is not executing above a thread that is RUNNING.
> >
> >
> > ### Eclipse Workspace Patch 1.0
> > #P src
> > Index: gdb/thread.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/thread.c,v
> > retrieving revision 1.138
> > diff -u -r1.138 thread.c
> > --- gdb/thread.c 10 Mar 2011 18:33:59 -0000 1.138
> > +++ gdb/thread.c 17 Apr 2011 12:40:15 -0000
> > @@ -471,6 +471,7 @@
> > {
> > struct thread_info *tp;
> > struct thread_info *tp_running = NULL;
> > + struct thread_info *tp_not_executing = NULL;
> >
> > for (tp = thread_list; tp; tp = tp->next)
> > if (ptid_get_pid (tp->ptid) == pid)
> > @@ -478,8 +479,15 @@
> > if (tp->state_ == THREAD_STOPPED)
> > return tp;
> > else if (tp->state_ == THREAD_RUNNING)
> > - tp_running = tp;
> > + {
> > + if (tp->executing_)
> > + tp_running = tp;
> > + else
> > + tp_not_executing = tp;
> > + }
> > }
> > + if (tp_not_executing)
> > + return tp_not_executing;
> >
> > return tp_running;
> > }
> >
>
> Assuming something like this compiles & works:
>
> struct thread_info *
> any_live_thread_of_process (int pid)
> {
> struct thread_info *tp;
> struct thread_info *tp_executing = NULL;
>
> for (tp = thread_list; tp; tp = tp->next)
> if (tp->state_ != THREAD_EXITED && ptid_get_pid (tp->ptid) == pid)
> {
> if (tp->executing_)
> tp_executing = tp;
> else
> return tp;
> }
>
> return tp_executing;
> }
Thanks for the updated code, I've made the change and confirmed
no regresssions.
> the patch is okay everywhere with that change. Bonus points if you
> s/already stopped/not executing/ in gdbthread.h:
>
> /* Returns any non-exited thread of process PID, giving preference for
> already stopped threads. */
> extern struct thread_info *any_live_thread_of_process (int pid);
Done that too.
Committed to HEAD, 7_3 and 7_2 as per below.
Thanks!
2011-04-19 Marc Khouzam <marc.khouzam@ericsson.com>
* thread.c (any_live_thread_of_process): Prioritize threads
that are not executing.
* gdbthread.h (any_live_thread_of_process): Update comment
as per above change.
### Eclipse Workspace Patch 1.0
#P src
Index: gdb/gdbthread.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbthread.h,v
retrieving revision 1.63
diff -u -r1.63 gdbthread.h
--- gdb/gdbthread.h 21 Feb 2011 23:40:46 -0000 1.63
+++ gdb/gdbthread.h 19 Apr 2011 15:15:39 -0000
@@ -287,7 +287,7 @@
extern struct thread_info *any_thread_of_process (int pid);
/* Returns any non-exited thread of process PID, giving preference for
- already stopped threads. */
+ not executing threads. */
extern struct thread_info *any_live_thread_of_process (int pid);
/* Change the ptid of thread OLD_PTID to NEW_PTID. */
Index: gdb/thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.138
diff -u -r1.138 thread.c
--- gdb/thread.c 10 Mar 2011 18:33:59 -0000 1.138
+++ gdb/thread.c 19 Apr 2011 15:15:39 -0000
@@ -470,18 +470,18 @@
any_live_thread_of_process (int pid)
{
struct thread_info *tp;
- struct thread_info *tp_running = NULL;
+ struct thread_info *tp_executing = NULL;
for (tp = thread_list; tp; tp = tp->next)
- if (ptid_get_pid (tp->ptid) == pid)
+ if (tp->state_ != THREAD_EXITED && ptid_get_pid (tp->ptid) == pid)
{
- if (tp->state_ == THREAD_STOPPED)
+ if (tp->executing_)
+ tp_executing = tp;
+ else
return tp;
- else if (tp->state_ == THREAD_RUNNING)
- tp_running = tp;
}
- return tp_running;
+ return tp_executing;
}
/* Print a list of thread ids currently known, and the total number of