for review: interface to external opium-style debugger

[Zoltan Somogyi]

>  # The following rules automatically build the library documentation
>  # by extracting the module interfaces from the library source code.
> +# Note that the debugger_interface.m module is just an implementation
> +# detail of the library, so it is not documented.
>  
>  library-menu.texi: $(LIBRARY_DIR)/*.m
> -	{ \
> -	echo ""; \
> -	for file in $(LIBRARY_DIR)/*.m; do \
> -		echo "* `basename $$file .m`::"; \
> -	done; \
> +	{								\
> +	echo "";							\
> +	for filename in $(LIBRARY_DIR)/*.m; do				\
> +		case $$filename in					\
> +			$(LIBRARY_DIR)/debugger_interface.m)		\
> +				;;					\
> +			*)						\
> +				echo "* `basename $$filename .m`::"; 	\
> +				;;					\
> +		esac;							\
> +	done;								\
>  	} > library-menu.texi

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.

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). Together with the lack of
anything exported from the module (to Mercury programs, that is), this
suggests that the standard library is not the right place for this code.

> +% The debugger_response type is used for response sent
> +% to the debugger process from the Mercury program being debugged.
> +% This type would need to be extended.
> +:- type debugger_response
> +	% responses to hello
> +	--->	hello_reply	% yes, I'm here
> +	% responses to forward_move
> +	;	forward_move_match_found
> +	;	forward_move_match_not_found
> +	% responses to current 
> +	;	current(
> +			maybe(event_number),
> +			maybe(call_number),
> +			maybe(depth_number),
> +			maybe(trace_port_type),
> +			maybe(string),	% module name
> +			maybe(string),	% pred name
> +			maybe(arity),
> +			maybe(int),	% mode number
> +			maybe(determinism),
> +			maybe(list(univ)), % the arguments
> +				% XXX we could provide better ways of
> +				% matching on arguments
> +			maybe(goal_path_string)

I don't understand this 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.

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

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

> --- mercury_trace.h	1998/02/03 08:17:26	1.5
> +++ mercury_trace.h	1998/02/06 07:40:44

> +#ifdef MR_USE_DEBUGGER
> +/*
> +** MR_trace_end() is called from mercury_runtime_terminate()
> +** when the debuggee programs is exiting.
> +*/
> +extern	void	MR_trace_end(void);
> +#endif

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.

mercury_wrapper.c:
> +#ifdef MR_USE_DEBUGGER
> +	MR_trace_end();
> +#endif

Ditto.

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.

Zoltan.