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

[Gary Benson <gbenson at redhat dot com>]

All issues not explicitly replied to below have been fixed as
suggested.

Jan Kratochvil wrote:
> On Thu, 16 May 2013 16:48:38 +0200, Gary Benson wrote:
> > diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> > index 4e09472..6b4bdb0 100644
> [...]
> > -/* Implement the "current_sos" target_so_ops method.  */
> > +/* Read the full list of currently loaded shared objects directly from
> > +   the inferior.  */
> >  
> >  static struct so_list *
> > -svr4_current_sos (void)
> > +svr4_current_sos_direct (struct svr4_info *info)
> 
> Meaning of 'direct' seems ambiguous/unclear to me, maybe 'uncached'?
> Just a hint, OK even with 'direct'.

Naming this function was the final thing I did before mailing the
original patch... it has been svr4_current_sos_XXX for some time :)

When I started writing the incremental code I thought of the solibs
as being cached by GDB, but a few versions later I didn't like the
terminology and renamed all the variables.   The stuff about caching
in the comments here was a hangover from that.

What I've done in the attached patch is to update the comments so
they don't mention caching.  This should hopefully make what is
happening clearer.  I've left the name as "direct" here, though
only because I can't think of anything better--I'm not especially
happy with the name myself.

Would "svr4_current_sos_1" be acceptable, or is the "_1" naming
discouraged?

> > +/* Populate the shared object list by reading the entire list of
> > +   shared objects from the inferior.  Returns nonzero on success.  */
> > +
> > +static int
> > +solist_update_full (struct svr4_info *info)
> > +{
> > +  svr4_free_library_list (&info->solib_list);
> 
> I find it a bit fragily this way, I would find worth it here also:
>   info->solib_list = NULL;
> 
> As svr4_current_sos_direct is a pretty big function and it could
> somehow check/update even info->solib_list possibly in the future.
> 
> Maybe a matter of opinion a bit.

That's a good idea and I've implemented it.

> > +/* Update the shared object list starting from the link-map entry
> > +   passed by the linker in the probe's third argument.  Returns
> > +   nonzero if the list was successfully updated, or zero to indicate
> > +   failure.  */
> > +
> > +static int
> > +solist_update_incremental (struct svr4_info *info, CORE_ADDR lm)
> > +{
> > +  struct so_list *tail;
> > +  CORE_ADDR prev_lm;
> > +
> > +  /* Fall back to a full update if we haven't read anything yet.  */
> > +  if (info->solib_list == NULL)
> > +    return 0;
> > +
> > +  /* Fall back to a full update if we are using a remote target
> > +     that does not support incremental transfers.  */
> > +  if (info->using_xfer && !target_augmented_libraries_svr4_read())
> > +    return 0;
> > +
> > +  /* Walk to the end of the list.  */
> > +  for (tail = info->solib_list; tail->next; tail = tail->next);
[...]
> > +  prev_lm = tail->lm_info->lm_addr;
> > +
> > +  /* Read the new objects.  */
> > +  if (info->using_xfer)
> > +    {
> > +      struct svr4_library_list library_list;
> > +      char annex[64];
> > +
> > +      xsnprintf (annex, sizeof (annex), "start=%lx;prev=%lx", lm, prev_lm);
[...]
> > +      if (!svr4_current_sos_via_xfer_libraries (&library_list, annex))
> > +	return 0;
> > +
> > +      tail->next = library_list.head;
> > +    }
> > +  else
> > +    {
> > +      struct so_list **link = &tail->next;
> > +
> > +      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.

> > +static void
> > +svr4_handle_solib_event (void)
> > +{
[...]
> 
> You could just always do { do_cleanups; return; } IMO and avoid the
> goto but up to you.

I have done this.  The original code after the goto was more complex,
but now it is a single line this is a no-brainer.

> > +static int
> > +svr4_update_solib_event_breakpoint (struct breakpoint *b, void *arg)
> > +{
[...]
> 
> It seems a bit suspicious that no update_global_location_list* is
> called.  Why is it safe?

I didn't know about update_global_location_list.  I've updated this
code to use enable_breakpoint and disable_breakpoint.

> > @@ -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.

Thanks,
Gary

Attachment: rtld-probes-5-main-changes.patch
Description: Text document