This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA 5/7 take 2] Improved linker-debugger interface
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: gdb-patches at sourceware dot org
- Date: Sat, 25 May 2013 23:05:35 +0200
- Subject: Re: [RFA 5/7 take 2] Improved linker-debugger interface
- References: <20130516144340 dot GA2105 at blade dot nx> <20130516144838 dot GF2105 at blade dot nx> <20130517185002 dot GA10447 at host2 dot jankratochvil dot net> <20130524083044 dot GC4602 at blade dot nx>
On Fri, 24 May 2013 10:30:44 +0200, Gary Benson wrote:
> Jan Kratochvil wrote:
> > Meaning of 'direct' seems ambiguous/unclear to me, maybe 'uncached'?
> > Just a hint, OK even with 'direct'.
[...]
> Would "svr4_current_sos_1" be acceptable, or is the "_1" naming
> discouraged?
*_direct seems already more explanatory to me than *_1.
Your new function comment explains it better:
/* Read the full list of currently loaded shared objects directly
from the inferior, without referring to any libraries read and
stored by the probes interface. */
> > > + if (!svr4_read_so_list (lm, prev_lm, &link, 0))
> >
> > You should set the last IGNORE_FIRST parameter properly. While
> > glibc has "" there AFAIK some OSes like Solaris may have some valid
> > pathname there which would confuse GDB listing the executable also
> > as a shared library.
>
> I set IGNORE_FIRST to zero here because for this particular call
> svr4_read_so_list never sees the first element in the list. If
> solist_update_incremental is called at the top of the list then
> the "if (info->solib_list == NULL) return 0;" at the top of
> solist_update_incremental causes it to defer to solist_update_full.
> That uses svr4_current_sos_direct, which does set IGNORE_FIRST
> correctly.
OK, that makes sense. But it does not seem obvious to me, add there
a comment please, something like:
/* The IGNORE_FIRST parameter does not need to be used as this point of code
doing incremental fetch is never used for the first element of the list.
solist_update_full is called for the first element instead. */
> > > @@ -1460,6 +2032,8 @@ enable_break (struct svr4_info *info, int from_tty)
> > > info->interp_text_sect_low = info->interp_text_sect_high = 0;
> > > info->interp_plt_sect_low = info->interp_plt_sect_high = 0;
> > >
> > > + free_probes_table (info);
> >
> > Why is this one needed and free_solib_list is not needed?
>
> free_solib_list is called in svr4_solib_create_inferior_hook.
> I originally had the two calls together, in enable_break, but
> doing it that way caused breakpoint resetting errors when the
> inferior was restarted. There is a check for this in
> info-shared.exp.
When I add free_solib_list call there info-shared.exp still PASSes, even if
I also remove the free_solib_list call from svr4_solib_create_inferior_hook.
I do not see why free_solib_list should be called at one place and
free_probes_table at other place, if it is really needed it would be worth
a comment as I do not understand now why.
The difference between placing it to svr4_solib_create_inferior_hook vs.
placing it to enable_break is only in svr4_relocate_main_executable being
called in the meantime. I may miss something but I do not see how
svr4_relocate_main_executable could be dependent on the shared library list
state.
It is pre-approved if you call free_probes_table and free_solib_list together,
otherwise I would like another reply mail about it.
> +/* 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));
> +
> + if (src->lm_info != NULL)
svr4 lm_info can never be NULL.
If you refer to
[patch] Fix crash in svr4_clear_so
http://sourceware.org/ml/gdb-patches/2013-05/msg00792.html
that could happen only when creating struct so_list in svr4_read_so_list but
here struct so_list has to be already finished.
Other solib-svr4.c code also assumes so_list->lm_info is not NULL.
> + {
> + 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;
> +}
[...]
> static int
> -svr4_current_sos_via_xfer_libraries (struct svr4_library_list *list)
> +svr4_current_sos_via_xfer_libraries (struct svr4_library_list *list,
> + const char *annex)
> {
> char *svr4_library_document;
> int result;
> struct cleanup *back_to;
>
> + gdb_assert (annex == NULL || target_augmented_libraries_svr4_read());
GNU Coding Standards:
gdb_assert (annex == NULL || target_augmented_libraries_svr4_read ());
> +
> /* Fetch the list of shared libraries. */
> svr4_library_document = target_read_stralloc (¤t_target,
> TARGET_OBJECT_LIBRARIES_SVR4,
> - NULL);
> + annex);
> if (svr4_library_document == NULL)
> return 0;
>
[...]
> +/* Create and register solib event breakpoints. PROBES is an array
> + of NUM_PROBES elements, each of which is vector of probes. A
^^^
Maybe "An" but you know better, nitpick.
> + solib event breakpoint will be created and registered for each
> + probe. */
> +
> +static void
> +svr4_create_probe_breakpoints (struct gdbarch *gdbarch,
> + VEC (probe_p) **probes)
[...]
OK for commit with the changes above and depending on the free_probes_table
and free_solib_list enable_break issue resolution.
Thanks,
Jan