This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA 14/14] Changes to solib-svr4.c
- From: Sergio Durigan Junior <sergiodj at redhat dot com>
- To: gdb-patches at sourceware dot org
- Date: Wed, 17 Jul 2013 02:09:14 -0300
- Subject: Re: [RFA 14/14] Changes to solib-svr4.c
- References: <20130711103144 dot GA5707 at blade dot nx> <20130711104549 dot GO5707 at blade dot nx>
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