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

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

On Wednesday, July 18 2012, Jan Kratochvil wrote:

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

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

I will not argue.

-- 
Sergio