Re: Patch for 'next' fails to set internal bp in non-stop

[Pedro Alves <pedro at codesourcery dot com>]

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.

> (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.

> 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;
}

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);

Thanks!

-- 
Pedro Alves