Re: [PATCH 3/3] Per-inferior thread list, thread ranges/iterators, down with ALL_THREADS, etc.

[Tom Tromey <tom at tromey dot com>]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> As preparation for multi-target, this patch makes each inferior have
Pedro> its own thread list.

This is great.

Pedro> Lastly, the solution I settled with was to replace the ALL_THREADS /
Pedro> ALL_NON_EXITED_THREADS / ALL_INFERIORS macros with (C++20-like) ranges
Pedro> and iterators, such that you can instead naturaly iterate over
Pedro> threads/inferiors using range-for, like e.g,.:

This is my preferred approach as well.  I wouldn't mind if we got rid of
all the iteration macros, and (by default) changed callback-based
iteration to for-range with iterators.

I like filtered_iterator and safe_iterator quite a bit.  I think I had a
use for them already but just didn't think to do it this way.

Pedro> init_wait_for_inferior is currently responsible for discarding skipped
Pedro> inline frames, which had to be moved elsewhere.

Note that ASAN pointed out a use-after-free bug in
clear_inline_frame_state.  It can be called sometimes after the
thread_info has been deleted, so:

    return pid == state.thread->inf->pid;

... references freed memory.

I wonder if your patch fixes this.  If not, maybe I have a patch on my
asan branch (or maybe that was one of the ones I had to rewrite, I can't
recall at the moment).

Pedro>	* inline-frame.c (clear_inline_frame_state(thread_info*)): New
Pedro>	overload.
Pedro>	* inline-frame.h (clear_inline_frame_state(thread_info*)): New
Pedro>	overload.

I didn't see these in the patch.

Pedro> +/* Filter for filtered_iterator.  Filters out exited inferiors.  */
Pedro> +
Pedro> +struct exited_inferior_filter
Pedro> +{
Pedro> +  bool operator() (inferior *inf)
Pedro> +  {
Pedro> +    return (inf->pid != 0);

Nit: the parens.  There were some other instances of this.  Maybe we
don't care.

Pedro> +  /* Returns a range adapter covering the inferior's non-exited
Pedro> +     threads.  Used like this:
Pedro> +
Pedro> +       for (thread_info *thr : inf->non_exited_threads ())
Pedro> +	 [...]
Pedro> +  */
Pedro> +  inf_non_exited_threads_range non_exited_threads ()
Pedro> +  { return inf_non_exited_threads_range (this->thread_list); }
Pedro> +
Pedro> +  inline safe_inf_threads_range threads_safe ()
Pedro> +  { return safe_inf_threads_range (this->thread_list); }

Nit: threads_safe should have an intro comment.

Pedro> +#include "inferior-iter.h"

I've been using "common/..." but it seems to vary in gdb.

Pedro> +void
Pedro> +all_threads_iterator::advance ()
Pedro> +{
Pedro> +  /* The loop below is written in the natural way as-if we'd always
Pedro> +     start at the beginning of the inferior list.  This fast forwards
Pedro> +     the algorithm to the actual current position.  */
Pedro> +  goto start;

I did a double take on this one, but it does make sense.

Tom