Re: [RFA 14/14] Changes to solib-svr4.c

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Thursday, July 11 2013, Gary Benson wrote:

> This patch modifies svr4_free_so to free the containing so_list
> object, and adds the acquire/release logic described in [1].
> Note that there is a more programmatic description of this in
> the final hunk of this patch.
>
> [1] http://sourceware.org/ml/gdb-patches/2013-07/msg00298.html

Thanks for the patch (and nice work), Gary.  I looked at them, and as
far as I understood the modifications, they seem fine.  I only have a
few comments for this one.

> 2013-07-10  Gary Benson  <gbenson@redhat.com>
>
> 	* solib-svr4.c (struct lm_info): New fields "copy" and "acquired".
> 	(svr4_free_so): Add acquired copy logic.  Free the containing
> 	so_list when freeing is required.
> 	(svr4_copy_library_list): Remove.
> 	(svr4_current_sos): Replace call to svr4_copy_library_list with
> 	acquired copy logic.
>
> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index 6f8d0ef..cab46f1 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -72,6 +72,15 @@ struct lm_info
>  
>      /* Values read in from inferior's fields of the same name.  */
>      CORE_ADDR l_ld, l_next, l_prev, l_name;
> +
> +    /* A copy of the solib containing this lm_info, if one exists.
> +       See comment in svr4_current_sos for more information.  */
> +    struct so_list *copy;
> +
> +    /* Nonzero if the solib containing this lm_info is an acquired
> +       copy of another solib.  See comment in svr4_current_sos for
> +       more information.  */
> +    unsigned int acquired : 1;
>    };
>  
>  /* On SVR4 systems, a list of symbols in the dynamic linker where
> @@ -1055,7 +1064,29 @@ struct svr4_library_list
>  static void
>  svr4_free_so (struct so_list *so)
>  {
> +  if (so->lm_info)
> +    {
> +      /* If this library is an acquired copy then release it. */
> +      if (so->lm_info->acquired)
> +	{
> +	  so->lm_info->acquired = 0;
> +	  return;
> +	}
> +
> +      /* If the library has an acquired copy then unhook it.  */
> +      if (so->lm_info->copy && so->lm_info->copy->lm_info->acquired)
> +	{
> +	  so->lm_info->copy->lm_info->acquired = 0;
> +	  so->lm_info->copy = NULL;
> +	}
> +
> +      /* If the library has an unacquired copy then free it.  */
> +      if (so->lm_info->copy)
> +	svr4_free_so (so->lm_info->copy);

First of all, the implicit checking for != NULL.  I've seen this topic
being discussed many times, and I hard-wired in my brain that it is not
a good GDB coding style to make them.  So, in this particular case, you
would write:

  if (so->lm_info->copy != NULL)

Anyway, this is the kind of thing I try to do in my own patches, so I
thought it was good mentioning to you as well.  But I definitely won't
bikeshed about it.

Secondly, I found the way you organized the if's confusing.  What do you
think of the following alternative?

  if (so->lm_info->copy != NULL)
    {
      if (so->lm_info->copy->lm_info->acquired)
        {
          /* If the library has an acquired copy then unhook it.  */
          so->lm_info->copy->lm_info->acquired = 0;
          so->lm_info->copy = NULL;
        }
      else
        {
          /* If the library has an unacquired copy then free it.  */
          svr4_free_so (so->lm_info->copy);
        }
    }

Maybe it's me, but I find it easier to read and understand.

> +    }
> +
>    xfree (so->lm_info);
> +  xfree (so);
>  }
>  
>  /* Implement target_so_ops.clear_so.  */
> @@ -1083,34 +1114,6 @@ svr4_free_library_list (void *p_list)
>      }
>  }
>  
> -/* Copy library list.  */
> -
> -static struct so_list *
> -svr4_copy_library_list (struct so_list *src)
> -{
> -  struct so_list *dst = NULL;
> -  struct so_list **link = &dst;
> -
> -  while (src != NULL)
> -    {
> -      struct so_list *new;
> -
> -      new = xmalloc (sizeof (struct so_list));
> -      memcpy (new, src, sizeof (struct so_list));
> -
> -      new->lm_info = xmalloc (sizeof (struct lm_info));
> -      memcpy (new->lm_info, src->lm_info, sizeof (struct lm_info));
> -
> -      new->next = NULL;
> -      *link = new;
> -      link = &new->next;
> -
> -      src = src->next;
> -    }
> -
> -  return dst;
> -}
> -
>  #ifdef HAVE_LIBEXPAT
>  
>  #include "xml-support.h"
> @@ -1470,7 +1473,62 @@ svr4_current_sos (void)
>    /* If the solib list has been read and stored by the probes
>       interface then we return a copy of the stored list.  */
>    if (info->solib_list != NULL)
> -    return svr4_copy_library_list (info->solib_list);
> +    {
> +      struct so_list *so, *result, **link = &result;
> +
> +      /* To avoid copying each solib each time the list is updated,
> +	 we retain pointers to the copies and mark each copy as
> +	 "acquired" before returning the list to update_solib_list.
> +	 Then, for each copy:
> +
> +         - If the solib has already been added to the program space's
> +           so_list then update_solib_list will free the copy we
> +           returned using free_so.  This releases the acquired copy
> +           for re-use by the next call to svr4_current_sos.
> +
> +         - If the solib has not been added to the program space's
> +           so_list then update_solib_list will not free the copy we
> +           returned.  The next call to svr4_current_sos will note that
> +           the copy is still acquired and create a fresh copy to
> +           return.
> +
> +         This ensures no solib is copied more than twice. */
> +      for (so = info->solib_list; so; so = so->next)
> +	{
> +	  gdb_assert (so->lm_info != NULL);
> +
> +	  /* If the library has an acquired copy then unhook it.  */
> +	  if (so->lm_info->copy && so->lm_info->copy->lm_info->acquired)
> +	    {
> +	      so->lm_info->copy->lm_info->acquired = 0;
> +	      so->lm_info->copy = NULL;
> +	    }
> +
> +	  /* If this library does not have a copy then create one.  */
> +	  if (so->lm_info->copy == NULL)
> +	    {
> +	      struct so_list *copy;
> +
> +	      copy = XZALLOC (struct so_list);
> +	      memcpy (copy, so, sizeof (struct so_list));
> +
> +	      copy->lm_info = xmalloc (sizeof (struct lm_info));
> +	      memcpy (copy->lm_info, so->lm_info, sizeof (struct lm_info));
> +
> +	      so->lm_info->copy = copy;
> +	    }
> +
> +	  /* Mark the copy as acquired and link it into the result.  */
> +	  gdb_assert (!so->lm_info->copy->lm_info->acquired);
> +	  so->lm_info->copy->lm_info->acquired = 1;
> +
> +	  so->lm_info->copy->next = NULL;
> +	  *link = so->lm_info->copy;
> +	  link = &so->lm_info->copy->next;
> +	}
> +
> +      return result;
> +    }
>  
>    /* Otherwise obtain the solib list directly from the inferior.  */
>    return svr4_current_sos_direct (info);

The rest (including the rest of the series) looks OK, as far as I have
seen.

Thanks,

-- 
Sergio