[MI] core awareness

[Pedro Alves]

On Saturday 09 January 2010 12:40:39, Vladimir Prus wrote:
> On Friday 08 January 2010 23:30:20 Pedro Alves wrote:
> 
> > > The primary problem is that when we process a stop reply
> > > for a new thread, the thread is not created and added to gdb
> > > thread table yet, so it's not possible to set its info->private.
> > > I don't see any way around this. 
> > 
> > The remote_notice_new_inferior calls always return
> > with the possibly new thread added to the list already.  From
> > the patched code:
> > 
> > remote_threads_info:
> > ...
> > 		      remote_notice_new_inferior (item->ptid, running);
> > 
> > 		      info = find_thread_ptid (item->ptid);
> 
> Oh, I did not realize this. I've simplified the code with this
> assumption in mind, and killed the two globals.
> 
> > You can, for example, pass the core to remote_notice_new_inferior,
> > and have remote_notice_new_inferior itself update the core field.
> 
> I took a slightly different approach, since passing core to a function
> whose purpose is to handle appearance of a new thread would be somewhat
> inconsistent.
> 
> > > -several threads in the list.
> > > +several threads in the list.  The @var{core} field reports the
> > > +processor core on which the stop   event has happened.  This field may be absent
> > > +if such information is not available.
> > 
> > s/stop   event/stop event/
> > 
> > 
> > Formatting here is wrong:
> > 
> > > +                         if (!info->private) {
> > > +                           info->private = (struct private_thread_info *)
> > > +                             xmalloc (sizeof (struct private_thread_info));
> > > +                           info->private_dtor = free_private_thread_info;
> > > +                         }
> > 
> > 
> > ( If you write that as `info->private = xmalloc (sizeof (*info->private))'
> > it's a shorter line. )
> > 
> > 
> > Also:
> > 
> > $ quilt refresh
> > Warning: trailing whitespace in line 118 of gdb/thread.c
> > Warning: trailing whitespace in lines 602,603,606 of gdb/target.h
> > Refreshed patch core3.diff
> 
> I've fixed the above formatting issues.

And added others :-) :

 $ quilt refresh
 Warning: trailing whitespace in lines 1387,1392 of gdb/remote.c
 Refreshed patch core4.diff

> 
> 
> > > For a start, I certainly find that a single packet that returns all information
> > > about threads -- which a frontend might be requesting on each step -- is a good
> > > idea regardless of "core awareness". As for invalidation, I think it's reasonable
> > > to fetch information about all threads, just because the time to get that information
> > > is probably less than communication latency for a single packet.
> > 
> > Right, agreed on that then.  Good that it's considered.
> > 
> > > 
> > > So, for avoidance of doubt, how about this scheme:
> > > - core information of each thread is invalidated when that thread is resumed
> > >   (where resumption is reported by the 'target_resumed' observer)
> > > - remote_core_of_thread checks if it's asked for a core of the most recently
> > >   stopped thread. If so, it reports it. Otherwise, it refreshes the thread list, 
> > >   and then reports core it has fetched
> > > - this all is for all-stop only. For non-stop, we get stop packet of each stopped
> > >   thread and therefore remote_core_of_thread never needs to fetch information
> > >   about all threads.
> > > 
> > > If this sounds OK, I'll implement such a scheme.
> > 
> > Sounds good, although I still don't see why you need to special
> > case the last-reported-thread.  One concern about non-stop and
> > running threads though.  Looking at the new documentation of
> > the new target interface makes it clearer what I'm talking about:
> > 
> >     /* Return the core that thread PTID is on.  For a stopped thread, should 
> >        return the core the thread was last running on.  For a running thread, 
> >                                                         ^^^^^^^^^^^^^^^^^^^^^
> >        should return one of the cores that the thread was running between
> >        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >        the call to this function and return -- and if it was running on
> >        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >        several cores, any other may be returned.  
> >        If the core cannot be determined -- either for the specified thread, or
> >        right now, or in this debug session, or for this target -- return -1.  */
> > 
> >    int (*to_core_of_thread) (struct target_ops *, ptid_t ptid);
> > 
> > 
> > Specifically, the underlined bit.  If we look at the current 
> > remote.c implementation of that method,
> > 
> >  static int
> >  remote_core_of_thread (struct target_ops *ops, ptid_t ptid)
> >  {
> >    struct thread_info *info = find_thread_ptid (ptid);
> >    if (info && info->private)
> >      return info->private->core;
> >    else if (ptid_equal (ptid, thread_of_last_core))
> >      return last_core;
> >    return -1;
> >  }
> > 
> > we see that there's nothing here that considers the fact
> > that `info->private->core' gets stale real quick, and does
> > not actually represent "one of the cores that the thread
> > was running between the call to this function and return".
> > This is relying on the callers of target_core_of_thread
> > having refreshed the thread list, but there's
> > no reason a native target would need to update the
> > thread list to query the core of a single thread
> > when it has direct access to that info.
> > 
> > So, we either update the documentation of
> > target_core_of_thread's assumptions to match reality,
> > which feels a bit hackish given that it is a remote.c
> > related assumption, or we change the remote
> > implementation to refetch the data if the thread is
> > running, which has the problem that with the
> > current API, for every running thread, you'd re-update
> > the whole thread list (and this is what made me
> > wonder before how would you consider the protocol
> > should handle updating the core data of a
> > single thread).
> 
> I guess we should recall what uses core information has. First,
> it's necessary to inform the user what thread is running on what
> processor. This is a 'wholesale' operation. Second, it's necessary
> to report the core of the thread that has just stopped. I do not see
> a use case that requires to find the core of the individual running 
> thread. Furthermore, iterating over all threads, calling
> target_core_of_thread, will always be inefficient.
> 
> So, maybe we should not use target method at all. Instead:
> 
> - Introduce thread_get_core function in thread.c. That thread will
>   be documented as returning the core a given thread was last noticed
>   running on by GDB, with that information updated either by
>   update_thread_list or when a thread stops. We'd need a new field
>   inside thread_info to keep this information.
> - Make linux native update core information for all threads when
>   listing them.

Sounds good to me.  Note that linux-nat.c doesn't list threads,
linux-thread-db.c does.

> What I am primarily trying to avoid is introducing a packet to get
> core information for a single running thread. That will be slow,
> and such functionality is not required by anything at present.

That's fine.  My gripe is in the disconnect between the
documentation of what the method should do, and what it
actually does.

-- 
Pedro Alves