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.


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

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.

> > Well, while I presumably could have split /proc/cpuinfo on empty lines,
> > and somehow print that in 'info os cores' I am unsure if we can promise
> > any fields to be present in the output, given that man:proc does not
> > mention any field except of "processor" and "bogomips". So, all that
> > "info os cores" can meaningfully print is the numeric ids of cores,
> > and this does not add very much to the information already available
> > to the frontend.
> 
> E.g., if all threads in a system are pinned to / running on only one
> of multiple cores, the frontend does not have have any way to know
> about all the cores on the target system.  No way to show to the
> user 40 threads on core 1, and 0 on core 2.  It simply has no
> info about core 2.  That's fine, I was only wondering if you
> had considered it; we can add this at any time.

Yes, I think we can add it later, if needed.

- Volodya