for review: interface to external opium-style debugger

[Fergus Henderson]

Hi Zoltan,

Thanks for your review.

On 16-Feb-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> > +# Note that the debugger_interface.m module is just an implementation
> > +# detail of the library, so it is not documented.
> 
> I don't like the idea of putting something in the standard library
> and then hiding it. It may be better to make this module a new library,
> one that is included in the executable by the linker when necessary.

Hmm.  On the one hand I'm inclined to agree with you;
on the other, it seems like overkill to have a whole new
library if it only contains one module.

How about if we leave it as is for the moment, but split it up
if/when the stuff in debugger_interface.m becomes complicated
enough to warrant being in more than one module?

I suppose we could make a new subdirectory (with corresponding library)
and put both debugger_interface.m and most of mercury_trace.c in it.
We could change MR_trace from being a function to being a function pointer,
and remove the other function pointers.

I guess this is probably the right way to go in the long term.
But changing all the infrastructure is quite a bit of work.
Do you think we should commit it as is for now,
or do you think we should wait until we've made the changes
outlined above before committing?

> In the medium term we would probably want the debugger_interface.m
> to be rewritten in C (in the short term, the inefficiency of its code
> can be ignored, while in the long term, new optimizations should allow
> the compiler to ged rid of the inefficiencies).

I'm not convinced that it _all_ needs to be so efficient.
Certainly the MR_trace() function needs to be fast in cases which
don't touch the socket, e.g. when doing a `skip' command,
and perhaps also for the `found_match' predicate (currently
written in Mercury) which tests whether the current port
matches the constraints specified by a `forward_move' request.
But for the cases which are going to involve socket I/O,
the overhead of doing it in Mercury does not seem to me to
be excessive, and the benefits (e.g. no need to write parsing
or formatting code) are substantial.

> > +	% responses to current 
> > +	;	current(
...
> > +			maybe(list(univ)), % the arguments
> > +				% XXX we could provide better ways of
> > +				% matching on arguments
> 
> I don't understand this comment.

Oops, that was a cut-and-paste error.  The fix is to delete the comment.

> > +		% XXX does the protocal require us to write out
> > +		% anything special first?
> > +		io__write(OutputStream, CurrentTraceInfo),
> > +		io__write_string(".\n")
> > +	;
> > +		{ error("output_current: current expected") }
> > +	).
> 
> You have to make sure that the protocol specifies how strings in arguments
> will be handled, since strings may contain the character sequence that
> terminates the answer. Whatever the answer, the above is unlikely to be right.

The code above assumes that the protocol is that requests/responses are passed
as Mercury/Prolog terms, in the format used by io__read and io__write.
That is, strings in the arguments are quoted using double-quotes,
double-quotes in the strings are escaped using backslashes, and terms
are terminated with a period followed by whitespace (e.g. \n).
This is a convenient protocol since the parsing and formatting
is trivial, on both the Prolog and the Mercury side.

However, now that I think about it, there is a problem.
The problem is that for a forward_move request, the argument
constraint is represented as a `match(list(univ))'; this
will cause problems because io__read does not support reading in univs.

But as noted in an XXX comment, `match(list(univ))' is not really
sufficient anyway -- what you really need is some way of doing partial matches.
A fix would be to change it to say `list(term)', and add code for
matching against terms.  (The quick prototype version of this code is

	match_with_term(Value, MatchTerm) :-
		type_to_term(Value, ValueTerm),
		map__init(Substitution0),
		term__unify(ValueTerm, MatchTerm, Substitution0, _Subst).

but for a usable version you need to interleave the computation
of term__unify and type_to_term.  Lazy evaluation would be nice here ;-)

The lack of support for univ in io__read would also cause problems
when reporting the current arguments, if the external debugger was
written in Mercury.  But the current plan is to write it in Prolog.

> > +#ifdef MR_USE_DEBUGGER
> > +	/* connect up to Opium */
> > +	if (!MR_socket_initialized) {
> > +		MR_init_socket();
> > +		MR_socket_initialized = TRUE;
> > +	}
> > +#endif
> 
> The trace function will be executed literally billions of times per run.
> You want to squeeze out every overhead possible. The above should be
> removed from MR_trace. The _init.c file should call another function
> in mercury_trace.c to initialize the socket.

OK.

> >  static void
> >  MR_trace_display(MR_trace_interact interact,
> > +	const MR_stack_layout_entry *layout,
> > +	MR_trace_port port, int seqno, int depth, const char *path)
> > +{
> > +#ifdef MR_USE_DEBUGGER
> > +	MR_debugger_step(interact, layout, port, seqno, depth, path);
> > +#else
> > +	MR_trace_display_user(interact, layout, port, seqno, depth, path);
> > +#endif
> > +}
> 
> Why do you want to hang this functionality on MR_trace_display?
> To reuse the current infrastructure of command modes in MR_trace?

Yes.

> You want to use a different set of command modes for trace analysis,
> and thus providing an alternate definition of MR_trace rather than
> MR_trace_display would be in order.
> 
> If you don't want to do this now, at least rename MR_trace_display.

OK.

> Whether mercury_trace.c defines MR_trace_end or not should not depend
> on a #define (which should be a variable test, as the diff itself said above).
> I suggest you always define MR_trace_end, but it should not do anything
> if we are not doing trace analysis.

OK.

The problem with making it a variable test rather than a #ifdef
is that the code uses sockets, which makes it less than completely
portable.  At least some of it needs to be #ifdef'd, and so it
seemed easier to #ifdef it all.  But I agree that it would be
nicer if we could keep the #ifdefs in the implementation rather
than in the interface, so I'll change this one.

> mkinit.c:
> > +	"#ifdef MR_USE_DEBUGGER\n"
> > +	"	MR_DI_output_current = ML_DI_output_current;\n"
> > +	"	MR_DI_found_match = ML_DI_found_match;\n"
> > +	"	MR_DI_read_request_from_socket = "
> > +				"ML_DI_read_request_from_socket;\n"
> > +	"#endif\n"
> 
> And again.

Here I don't agree.  If this was unconditional, it would force
debugger_interface.o to be linked into every program, which is
undesirable.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>   |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>   |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3         |     -- the last words of T. S. Garp.