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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

On Tue, 17 Jul 2012 20:11:07 +0200, Sergio Durigan Junior wrote:
> On Friday, July 13 2012, Gary Benson wrote:
> > +       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.

Could you clarify the 1-counting vs. 0-count in: evaluate_probe_argument,
compile_to_ax.  Maybe it is not fully clear even in
sym_evaluate_probe_argument, sym_compile_to_ax.
(It is already clear in probe_safe_evaluate_at_pc.)

OK to check it in as obvious if the new text is clear enough (0 <= x < count).


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

Currently the API comment defines "returning a value corresponding to it.".
There is no "or NULL if evaluation error occurs" or anything like that,
therefore it IMNSHO means the returned value is non-NULL.

Therefore I find correct for Gary to assume the returned value is non-NULL.


Thanks,
Jan