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

[Sergio Durigan Junior <sergiodj@redhat.com>]

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

> +/* Decide what action to take when the specified solib event probe is
> +   hit.  */
> +
> +static enum probe_action
> +solib_event_probe_action (struct probe_and_info *pi)
> +{
> +  enum probe_action action;
> +  int update;
> +  struct obj_section *os;
> +  unsigned probe_argc;
> +  struct svr4_info *info;
> +  CORE_ADDR debug_base;
> +
> +  action = pi->info->action;
> +  if (action == LM_CACHE_NO_ACTION || action == LM_CACHE_INVALIDATE)
> +    return action;
> +
> +  gdb_assert (action == LM_CACHE_RELOAD
> +	      || action == LM_CACHE_UPDATE_OR_RELOAD);
> +
> +  os = find_pc_section (pi->probe->address);
> +  if (os == NULL)
> +    return LM_CACHE_INVALIDATE;
> +
> +  /* 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.

Thanks,

-- 
Sergio