Re: [RFA 4/4] Improved linker-debugger interface

[Gary Benson <gbenson@redhat.com>]

Sergio Durigan Junior wrote:
> On Friday, July 13 2012, Gary Benson wrote:
> > diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> > index c111f04..ebb8c3c 100644
> > --- a/gdb/solib-svr4.c
> > +++ b/gdb/solib-svr4.c
[snip]
> > +  /* Check that an appropriate number of arguments has been supplied.
> > +     We expect:
> > +       arg1: Lmid_t lmid (mandatory)
> > +       arg2: struct r_debug *r_debug (mandatory)
> > +       arg3: struct link_map *new (optional, for incremental updates)  */
> 
> I guess you could rename the arguments listed here to 'arg0', 'arg1' and
> 'arg2', because `evaluate_probe_argument' takes these numbers as
> arguments.  Or you could explicitly say that here.  Otherwise it will
> confuse the reader, IMO.
> 
> > +  probe_argc = get_probe_argument_count (os->objfile, pi->probe);
> > +  if (probe_argc == 2)
> > +    action = LM_CACHE_RELOAD;
> > +  else if (probe_argc < 2)
> > +    return LM_CACHE_INVALIDATE;
> 
> This is OK...
> 
> > +  /* We only currently support the global namespace (PR gdb/11839).
> > +     If the probe's r_debug doesn't match the global r_debug then
> > +     this event refers to some other namespace and must be ignored.  */
> > +  info = get_svr4_info ();
> > +
> > +  /* Always locate the debug struct, in case it has moved.  */
> > +  info->debug_base = 0;
> > +  locate_base (info);
> > +
> > +  debug_base = value_as_address (evaluate_probe_argument (os->objfile,
> > +							  pi->probe, 1));
> 
> ...but what would happen if `evaluate_probe_argument' returned NULL?
> It's better to check this, because `value_as_address' calls `value_type'
> which does not check NULL pointers.
> 
> Currently, only the SystemTap backend is implemented, and if it returns
> NULL in this case it would be an error, but it's better to guard your
> code IMO.

Thank you for the review.  I've updated the comment and added NULL
checks (amongst other things) here:

  http://www.cygwin.com/ml/gdb-patches/2012-07/msg00333.html

Cheers,
Gary

-- 
http://gbenson.net/