Re: [PATCH] Finalize each cooked index separately

[Lancelot SIX <lsix@lancelotsix.com>]

Hi,

I have a couple of comments/suggestions inlined in the patch below.

> @@ -339,3 +248,98 @@ cooked_index_vector::finalize ()
>  	       return *a < *b;
>  	     });
>  }
> +
> +/* See cooked-index.h.  */
> +
> +cooked_index::range
> +cooked_index::find (gdb::string_view name, bool completing)
> +{
> +  wait ();
> +
> +  auto lower = std::lower_bound (m_entries.begin (), m_entries.end (),
> +				 name,
> +				 [=] (const cooked_index_entry *entry,
> +				      const gdb::string_view &n)
> +  {
> +    int cmp = strncasecmp (entry->canonical, n.data (), n.length ());
> +    if (cmp != 0 || completing)
> +      return cmp < 0;
> +    return strlen (entry->canonical) < n.length ();
> +  });
> +
> +  auto upper = std::upper_bound (m_entries.begin (), m_entries.end (),
> +				 name,
> +				 [=] (const gdb::string_view &n,
> +				      const cooked_index_entry *entry)
> +  {
> +    int cmp = strncasecmp (n.data (), entry->canonical, n.length ());
> +    if (cmp != 0 || completing)
> +      return cmp < 0;
> +    return n.length () < strlen (entry->canonical);
> +  });
> +
> +  return range (lower, upper);
> +}
> +
> +cooked_index_vector::cooked_index_vector (vec_type &&vec)
> +  : m_vector (std::move (vec))
> +{
> +  for (auto &idx : m_vector)
> +    idx->finalize ();
> +}
> +
> +/* See cooked-index.h.  */
> +
> +dwarf2_per_cu_data *
> +cooked_index_vector::lookup (CORE_ADDR addr)
> +{
> +  for (const auto &index : m_vector)
> +    {
> +      dwarf2_per_cu_data *result = index->lookup (addr);
> +      if (result != nullptr)
> +	return result;
> +    }
> +  return nullptr;
> +}
> +
> +/* See cooked-index.h.  */
> +
> +std::vector<addrmap *>
> +cooked_index_vector::get_addrmaps ()
> +{
> +  std::vector<addrmap *> result;
> +  for (const auto &index : m_vector)
> +    result.push_back (index->m_addrmap);
> +  return result;
> +}
> +
> +/* See cooked-index.h.  */
> +
> +cooked_index_vector::range
> +cooked_index_vector::find (gdb::string_view name, bool completing)
> +{
> +  std::vector<cooked_index::range> result_range;

Since the result_range's expected size is already known, it is possible
to pre-allocate the associated storage with

  result_range.reserve (m_vector.size);

> +  for (auto &entry : m_vector)
> +    result_range.push_back (entry->find (name, completing));
> +  return range (std::move (result_range));
> +}
> +
> +/* See cooked-index.h.  */
> +
> +const cooked_index_entry *
> +cooked_index_vector::get_main () const
> +{
> +  const cooked_index_entry *result = nullptr;
> +
> +  for (const auto &index : m_vector)
> +    {
> +      const cooked_index_entry *entry = index->get_main ();
> +      if (result == nullptr
> +	  || ((result->flags & IS_MAIN) == 0
> +	      && entry != nullptr
> +	      && (entry->flags & IS_MAIN) != 0))
> +	result = entry;
> +    }
> +
> +  return result;
> +}
> diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
> index ad4de05ba52..3b83250ef0f 100644
> --- a/gdb/dwarf2/cooked-index.h
> +++ b/gdb/dwarf2/cooked-index.h
> @@ -273,8 +318,10 @@ class cooked_index_vector : public dwarf_scanner_base
>    /* Return a range of all the entries.  */
>    range all_entries ()
>    {
> -    m_future.wait ();
> -    return { m_entries.begin (), m_entries.end () };
> +    std::vector<cooked_index::range> result_range;

Similar here, reserve can come in handy.

> +    for (auto &entry : m_vector)
> +      result_range.push_back (entry->all_entries ());
> +    return range (std::move (result_range));
>    }
>  
>    /* Look up ADDR in the address map, and return either the
> diff --git a/gdbsupport/range-chain.h b/gdbsupport/range-chain.h
> new file mode 100644
> index 00000000000..9bd6eb19480
> --- /dev/null
> +++ b/gdbsupport/range-chain.h
> @@ -0,0 +1,121 @@
> +/* A range adapter that wraps multiple ranges
> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef GDBSUPPORT_RANGE_CHAIN_H
> +#define GDBSUPPORT_RANGE_CHAIN_H
> +
> +/* A range adapter that presents a number of ranges as if it were a
> +   single range.  That is, iterating over a range_chain will iterate
> +   over each sub-range in order.  */
> +template<typename Range>
> +struct range_chain
> +{
> +  /* The type of the iterator that is created by this range.  */
> +  class iterator
> +  {
> +  public:
> +
> +    iterator (std::vector<Range> &ranges, size_t idx)
> +      : m_index (idx),
> +	m_ranges (ranges)
> +    {
> +      skip_empty ();
> +    }
> +
> +    bool operator== (const iterator &other) const
> +    {
> +      if (m_index != other.m_index || &m_ranges != &other.m_ranges)
> +	return false;
> +      if (m_current.has_value () != other.m_current.has_value ())
> +	return false;
> +      if (m_current.has_value ())
> +	return *m_current == *other.m_current;
> +      return true;
> +    }
> +
> +    bool operator!= (const iterator &other) const
> +    {
> +      return !(*this == other);
> +    }
> +
> +    iterator &operator++ ()
> +    {
> +      ++*m_current;
> +      if (*m_current == m_ranges[m_index].end ())
> +	{
> +	  ++m_index;
> +	  skip_empty ();
> +	}
> +      return *this;
> +    }
> +
> +    typename Range::iterator::value_type operator* () const
> +    {
> +      return **m_current;
> +    }
> +
> +  private:
> +    /* Skip empty sub-ranges.  If this finds a valid sub-range,
> +       m_current is updated to point to its start; otherwise,
> +       m_current is reset.  */
> +    void skip_empty ()
> +    {
> +      for (; m_index < m_ranges.size (); ++m_index)
> +	{
> +	  m_current = m_ranges[m_index].begin ();
> +	  if (*m_current != m_ranges[m_index].end ())
> +	    return;
> +	}
> +      m_current.reset ();
> +    }
> +
> +    /* Index into the vector indicating where the current iterator
> +       comes from.  */
> +    size_t m_index;
> +    /* The current iterator into one of the vector ranges.  If no
> +       value then this (outer) iterator is at the end of the overall
> +       range.  */
> +    gdb::optional<typename Range::iterator> m_current;
> +    /* Vector of ranges.  */
> +    std::vector<Range> &m_ranges;
> +  };
> +
> +  /* Create a new range_chain, transferring in a vector of
> +     sub-ranges.  */
> +  range_chain (std::vector<Range> &&ranges)
> +    : m_ranges (ranges)

Did you forget the std::move (ranges) here? Otherwise I think m_ranges
is initialized using a copy ctor instead of the intended move ctor.

Also, I am not certain range_chain should be restricted to only take
ownership of the vector or ranges.  This version should allow to take
lavlue or rvalue ref depending on the caller:

    template<typename Ranges>
    range_chain (Ranges &&ranges)
      : m_ranges (std::forward<Ranges> (ranges))
      { }

That being said, it is not needed in your patch and if someone needs
this it will be easy enough to add at that time.

> +  {
> +  }
> +
> +  iterator begin ()

I was wondering why the begin () and end () are not const.  Turns out
that they could become const if the following adjustments are done to
the iterator class:

- The range_chain::iterator::m_vector becomes a const member
- The range_chain::iterator::iterator ctor takes its vector parameter as
  a const ref.

I do not think there is case here for the iterator to change the
underlying vector, so making this const enforces it.

WDYT?

> +  {
> +    return iterator (m_ranges, 0);
> +  }
> +
> +  iterator end ()
> +  {
> +    return iterator (m_ranges, m_ranges.size ());
> +  }
> +
> +private:
> +
> +  /* The sub-ranges.  */
> +  std::vector<Range> m_ranges;
> +};
> +
> +#endif /* GDBSUPPORT_RANGE_CHAIN_H */
> -- 
> 2.34.1
> 

Best,
Lancelot.