Re: [RFA 5/7 take 2] Improved linker-debugger interface

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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 (&current_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