Re: [PATCH 2/3] Implement new features needed for handling SystemTap probes

[Tom Tromey <tromey@redhat.com>]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Thanks for the thorough and detailed review.  I have a few comments.

Jan> I believe there was intended some abstraction, more talked about it
Jan> elsewhere, breakpoint.c should only include some (nonexistent)
Jan> "probe.h", not "stap-probe.h".

What did you have in mind?

In the abstract, my concern about adding abstraction is that we aren't
sure it would be good for anything.  Then we can wind up with an
over-engineered solution.

But, this is an abstract concern; perhaps your idea is not susceptible
to this.

>> +  /* Swallow errors.  */

Jan> Could here be a reason why it is valid?

>> +  target_write_memory (address, bytes, TYPE_LENGTH (type));

Jan> And here again, why not to check errors?

I think we should warn.
Originally I thought there was just nothing sensible to do if we got an
error, but warning is better than silently ignoring it.

Jan> Here you leak PROV_PAT, PROBE_PAT and OBJ_PAT.

No, compile_rx_or_error makes a cleanup.

>> +  return result;
>> +}
>> +
>> +/* A qsort comparison function for stap_entry objects.  */
>> +
>> +static int
>> +compare_entries (const void *a, const void *b)
>> +{
>> +  const stap_entry *ea = a;
>> +  const stap_entry *eb = b;
>> +  int v;
>> +
>> +  v = strcmp (ea->probe->provider, eb->probe->provider);

Jan> handle_probe can create NULL probe->provider or NULL probe->name etc.  But
Jan> other code like this one will crash on it.

Jan> I believe such non-valid probes rather should be dropped during parsing.

I agree.

Jan> Moreover I would still more see to drop [patch 1/3], call just
Jan> compute_probe_arg which returns lazy lval_computed struct value *
Jan> which provides new struct lval_funcs member which can return
Jan> `struct expression *' and generic code can call gen_expr on its
Jan> own.  There is no need for special
Jan> internalvar_funcs-> compile_to_ax member.

This is a relic of an earlier implementation, which had two different
code paths.

Putting this into lval_funcs seems very roundabout to me.  First, it
means computing a value in the middle of compiling to AX.  But, when
compiling to AX we are not generally computing values.  The probe
argument might not even be valid at the time at which AX compilation is
done.  And, this means that we must always return a special computed
value for all probe arguments, just to make compilation work.  All these
gyrations provide no extra benefit, though.  Simply asking the
internalvar for an expression is simpler all around.

We could change the new method to return an expression; this doesn't
seem vital to me since it isn't used anywhere else, but I find it a
valid aesthetic choice.

Tom