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

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Mon, 12 Mar 2012 21:37:37 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
[...]
> 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.

I find important that user interface gets established in a way needing no
changes in the future, as discussed with Eli.

Including probe.h vs. stap-probe.h also seems more easier for readability of
the generic GDB code.  So called code encapsulation - not to complicate
unrelated code with the specific stap backend details visibility.

It would also make it more easy to implement breakpoints on UST addresses;
although I do not have ust.h available here to examine that possibility more.


> Jan> Here you leak PROV_PAT, PROBE_PAT and OBJ_PAT.
> 
> No, compile_rx_or_error makes a cleanup.

Yes but there is:
	discard_cleanups (cleanup);

which discards those cleanups.

IMO that discard_cleanups was meant for RESULT but not for PROV_PAT, PROBE_PAT
and OBJ_PAT - or I really still miss it?  There should be two cleanup trackers:

cleanup = make_cleanup (VEC_cleanup (stap_entry), &result);
cleanup_temps = compile_rx_or_error (&prov_pat, provider, _("Invalid provider regexp"));
compile_rx_or_error (&probe_pat, probe, _("Invalid probe regexp"));
compile_rx_or_error (&obj_pat, objname, _("Invalid object file regexp"));
[...]
do_cleanups (cleanup_temps);
discard_cleanups (cleanup);



> 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.
[...]
> 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.
+
> Simply asking the internalvar for an expression is simpler all around.

lval_computed value is not a real value, it is just a way how to implement the
needed functionality on top of existing API, without needing making that API
more rich.

I do not mind if you have considered the lval_computed way.


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

That's already detail, I was objecting more the whole [patch 1/3].


Thanks,
Jan