[PATCH] c++: memory corruption during name lookup w/ modules [PR99479]

Patrick Palka ppalka@redhat.com
Tue Mar 1 13:13:18 GMT 2022


On Thu, Feb 17, 2022 at 3:24 PM Patrick Palka <ppalka@redhat.com> wrote:
>
> name_lookup::search_unqualified uses a statically allocated vector
> in order to avoid repeated reallocation, under the assumption that
> the function can't be called recursively.  With modules however,
> this assumption turns out to be false, and search_unqualified can
> be called recursively as demonstrated by testcase in comment #19
> of PR99479[1] where the recursive call causes the vector to get
> reallocated which invalidates the reference held by the parent call.
>
> This patch makes search_unqualified instead use an auto_vec with 16
> elements of internal storage (since with the various libraries I tested,
> the size of the vector never exceeded 12).  In turn we can simplify the
> API of subroutines to take the vector by reference and return void.
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?

Ping.

>
> [1]: https://gcc.gnu.org/PR99479#c19
>
>         PR c++/99479
>
> gcc/cp/ChangeLog:
>
>         * name-lookup.cc (name_lookup::using_queue): Change to an
>         auto_vec (with 16 elements of internal storage).
>         (name_lookup::queue_namespace): Change return type to void,
>         take queue parameter by reference and adjust function body
>         accordingly.
>         (name_lookup::do_queue_usings): Inline into ...
>         (name_lookup::queue_usings): ... here.  As in queue_namespace.
>         (name_lookup::search_unqualified): Don't make queue static,
>         assume its incoming length is 0, and adjust function body
>         accordingly.
> ---
>  gcc/cp/name-lookup.cc | 62 +++++++++++++++----------------------------
>  1 file changed, 22 insertions(+), 40 deletions(-)
>
> diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> index 93c4eb7193b..5c965d6fba1 100644
> --- a/gcc/cp/name-lookup.cc
> +++ b/gcc/cp/name-lookup.cc
> @@ -429,7 +429,7 @@ class name_lookup
>  {
>  public:
>    typedef std::pair<tree, tree> using_pair;
> -  typedef vec<using_pair, va_heap, vl_embed> using_queue;
> +  typedef auto_vec<using_pair, 16> using_queue;
>
>  public:
>    tree name;   /* The identifier being looked for.  */
> @@ -528,16 +528,8 @@ private:
>    bool search_usings (tree scope);
>
>  private:
> -  using_queue *queue_namespace (using_queue *queue, int depth, tree scope);
> -  using_queue *do_queue_usings (using_queue *queue, int depth,
> -                               vec<tree, va_gc> *usings);
> -  using_queue *queue_usings (using_queue *queue, int depth,
> -                            vec<tree, va_gc> *usings)
> -  {
> -    if (usings)
> -      queue = do_queue_usings (queue, depth, usings);
> -    return queue;
> -  }
> +  void queue_namespace (using_queue& queue, int depth, tree scope);
> +  void queue_usings (using_queue& queue, int depth, vec<tree, va_gc> *usings);
>
>  private:
>    void add_fns (tree);
> @@ -1084,39 +1076,35 @@ name_lookup::search_qualified (tree scope, bool usings)
>  /* Add SCOPE to the unqualified search queue, recursively add its
>     inlines and those via using directives.  */
>
> -name_lookup::using_queue *
> -name_lookup::queue_namespace (using_queue *queue, int depth, tree scope)
> +void
> +name_lookup::queue_namespace (using_queue& queue, int depth, tree scope)
>  {
>    if (see_and_mark (scope))
> -    return queue;
> +    return;
>
>    /* Record it.  */
>    tree common = scope;
>    while (SCOPE_DEPTH (common) > depth)
>      common = CP_DECL_CONTEXT (common);
> -  vec_safe_push (queue, using_pair (common, scope));
> +  queue.safe_push (using_pair (common, scope));
>
>    /* Queue its inline children.  */
>    if (vec<tree, va_gc> *inlinees = DECL_NAMESPACE_INLINEES (scope))
>      for (unsigned ix = inlinees->length (); ix--;)
> -      queue = queue_namespace (queue, depth, (*inlinees)[ix]);
> +      queue_namespace (queue, depth, (*inlinees)[ix]);
>
>    /* Queue its using targets.  */
> -  queue = queue_usings (queue, depth, NAMESPACE_LEVEL (scope)->using_directives);
> -
> -  return queue;
> +  queue_usings (queue, depth, NAMESPACE_LEVEL (scope)->using_directives);
>  }
>
>  /* Add the namespaces in USINGS to the unqualified search queue.  */
>
> -name_lookup::using_queue *
> -name_lookup::do_queue_usings (using_queue *queue, int depth,
> -                             vec<tree, va_gc> *usings)
> +void
> +name_lookup::queue_usings (using_queue& queue, int depth, vec<tree, va_gc> *usings)
>  {
> -  for (unsigned ix = usings->length (); ix--;)
> -    queue = queue_namespace (queue, depth, (*usings)[ix]);
> -
> -  return queue;
> +  if (usings)
> +    for (unsigned ix = usings->length (); ix--;)
> +      queue_namespace (queue, depth, (*usings)[ix]);
>  }
>
>  /* Unqualified namespace lookup in SCOPE.
> @@ -1128,15 +1116,12 @@ name_lookup::do_queue_usings (using_queue *queue, int depth,
>  bool
>  name_lookup::search_unqualified (tree scope, cp_binding_level *level)
>  {
> -  /* Make static to avoid continual reallocation.  We're not
> -     recursive.  */
> -  static using_queue *queue = NULL;
> +  using_queue queue;
>    bool found = false;
> -  int length = vec_safe_length (queue);
>
>    /* Queue local using-directives.  */
>    for (; level->kind != sk_namespace; level = level->level_chain)
> -    queue = queue_usings (queue, SCOPE_DEPTH (scope), level->using_directives);
> +    queue_usings (queue, SCOPE_DEPTH (scope), level->using_directives);
>
>    for (; !found; scope = CP_DECL_CONTEXT (scope))
>      {
> @@ -1144,19 +1129,19 @@ name_lookup::search_unqualified (tree scope, cp_binding_level *level)
>        int depth = SCOPE_DEPTH (scope);
>
>        /* Queue namespaces reachable from SCOPE. */
> -      queue = queue_namespace (queue, depth, scope);
> +      queue_namespace (queue, depth, scope);
>
>        /* Search every queued namespace where SCOPE is the common
>          ancestor.  Adjust the others.  */
> -      unsigned ix = length;
> +      unsigned ix = 0;
>        do
>         {
> -         using_pair &pair = (*queue)[ix];
> +         using_pair &pair = queue[ix];
>           while (pair.first == scope)
>             {
>               found |= search_namespace_only (pair.second);
> -             pair = queue->pop ();
> -             if (ix == queue->length ())
> +             pair = queue.pop ();
> +             if (ix == queue.length ())
>                 goto done;
>             }
>           /* The depth is the same as SCOPE, find the parent scope.  */
> @@ -1164,7 +1149,7 @@ name_lookup::search_unqualified (tree scope, cp_binding_level *level)
>             pair.first = CP_DECL_CONTEXT (pair.first);
>           ix++;
>         }
> -      while (ix < queue->length ());
> +      while (ix < queue.length ());
>      done:;
>        if (scope == global_namespace)
>         break;
> @@ -1181,9 +1166,6 @@ name_lookup::search_unqualified (tree scope, cp_binding_level *level)
>
>    dedup (false);
>
> -  /* Restore to incoming length.  */
> -  vec_safe_truncate (queue, length);
> -
>    return found;
>  }
>
> --
> 2.35.1.193.g45fe28c951
>



More information about the Gcc-patches mailing list