[m-dev.] for review: interface tracing, part 1

[Fergus Henderson]

On 14-May-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> This change introduces interface tracing, and makes it possible to
> successfully bootstrap the compiler with tracing (either interface or full).

By the way, have you got some benchmarks for us -- how much bigger
and slower is it with interface tracing and full tracing?

> -:- pred globals__set_gc_method(globals::in, gc_method::in, globals::out)
> -	is det.
> -:- pred globals__set_tags_method(globals::in, tags_method::in, globals::out)
> -	is det.
> -:- pred globals__set_args_method(globals::in, args_method::in, globals::out)
> -	is det.
> -:- pred globals__set_prolog_dialect(globals::in, prolog_dialect::in,
> -	globals::out) is det.
> -:- pred globals__set_termination_norm(globals::in, termination_norm::in,
> -	globals::out) is det.
...
>  
> -globals__set_options(globals(_, B, C, D, E, F), Options,
> -	globals(Options, B, C, D, E, F)).
> -globals__set_gc_method(globals(A, _, C, D, E, F), GC_Method,
> -	globals(A, GC_Method, C, D, E, F)).
> -globals__set_tags_method(globals(A, B, _, D, E, F), TagsMethod,
> -	globals(A, B, TagsMethod, D, E, F)).
> -globals__set_args_method(globals(A, B, C, _, E, F), ArgsMethod,
> -	globals(A, B, C, ArgsMethod, E, F)).
> -globals__set_prolog_dialect(globals(A, B, C, D, _, F), PrologDialect,
> -	globals(A, B, C, D, PrologDialect, F)).
> -globals__set_termination_norm(globals(A, B, C, D, E, _), TerminationNorm,
> -	globals(A, B, C, D, E, TerminationNorm)).

Any particular reason for removing these?

options.m:
> +	io__write_string("\t--trace {minimal, interface, full}\n"),
> +	io__write_string("\t\tGenerate code that includes the specified level\n"), 
> +	io__write_string("\t\tof execution tracing.\n"),

These options are a bit ungrammatical.
`--trace interface' parses OK (although one could argue for
`--trace interfaces') with `trace' being an imperative verb
and `interface' a noun, but in each of `--trace full' and
`--trace minimal' there is a verb and an adjective, but no noun.

`--trace all' would be a reasonable grammatical alternative to
`--trace full', but I can't think of a reasonable replacement
for `--trace minimal'.

Actually `--trace full' and `--trace minimal' parse OK with
`trace' being a `noun'.  The problem is that `--trace interface'
is then inconsistent.  So another alternative would be to use
`--trace partial' instead of `--trace interface'.

stack_layout.m:
> +	{ Exported = no },	% With the new profiler, we will need to
> +				% set this to `yes' if the profiling option
> +				% is given and if the procedure is exported.
> +				% XXX

If the XXX here is about the comment above, then it should precede the
comment ("XXX With the new profiler, ...").
Otherwise it looks like someone added an XXX after that but didn't
explain why the XXX was there.

> +trace__prepare_for_call(TraceInfo, TraceCode) :-
> +	TraceInfo = trace_info(_CallNumLval, CallDepthLval, TraceType),
>  	trace__stackref_to_string(CallDepthLval, CallDepthStr),
>  	string__append_list(["MR_trace_reset_depth(", CallDepthStr, ");\n"],
> +		ResetDepthStmt),
> +	(
> +		TraceType = interface_trace(_),
> +		TraceCode = node([
> +			c_code("MR_trace_from_full = 0;\n") - "",
> +			c_code(ResetDepthStmt) - ""
> +		])
> +	;
> +		TraceType = full_trace,
> +		TraceCode = node([
> +			c_code("MR_trace_from_full = 1;\n") - "",
> +			c_code(ResetDepthStmt) - ""
> +		])
> +	).

I think it would be better to use FALSE and TRUE rather than 0 and 1.

There's a compilation time vs readability tradeoff here.
But I think the increase in compilation time is likely to be
pretty negligable and I think readability and consistency
are more important.

> +	(
> +		TraceType = full_trace,
> +		FlagStr = "1"				% meaning true
> +	;
> +		TraceType = interface_trace(CallFromFullLval),
> +		trace__stackref_to_string(CallFromFullLval, FlagStr)
> +	),

Ditto here.

> --- user_guide.texi	1998/04/27 04:03:21	1.121
> +++ user_guide.texi	1998/05/12 06:16:58
> @@ -1684,9 +1684,10 @@
>  do not assume the availability of GNU Make extensions.
>  This makes these files significantly larger.
>  
> - at item --generate-trace
> -Include code to generate an execution trace in the
> -C code output by the compiler.
> + at item --trace @var{level}
> +Generate code that includes the specified level of execution tracing.
> +The @var{level} should be one of
> + at samp{minimal}, @samp{interface} and @samp{full}.

The documentation should explain the effect of each of these
different tracing levels.

(In addition, the user guide should probably have a new section
on debugging.  But that can be committed as a seperate change.)

>  /*
> +** The layout structure for an internal label will have a field containing
> +** the label number of that label (or -1, if the internal label has no
> +** label number, being the entry label) only if we are using native gc.
> +*/
> +
> +#ifdef	NATIVE_GC
> +#define	UNKNOWN_INTERNAL_LABEL_FIELD	Integer f2;
> +#define	UNKNOWN_INTERNAL_LABEL_NUMBER	(Integer) -1,
> +#else
> +#define	UNKNOWN_INTERNAL_LABEL_FIELD
> +#define	UNKNOWN_INTERNAL_LABEL_NUMBER
> +#endif

Please indent stuff within #if or #ifdef by two spaces.

	#ifdef ...
	   #define ...
	#else
	   ...
	#endif

> Index: runtime/mercury_wrapper.c
...
>  	/* initialize the Mercury library */
>  	(*MR_library_initializer)();
> +
> +	/* now the real tracing starts */
> +	MR_trace_event_number = 0;
> +	MR_trace_call_seqno = 0;
> +	MR_trace_call_depth = 0;
> +	MR_trace_from_full = 1;
> +	MR_trace_enabled = saved_trace_enabled;

mercury_wrapper.c shouldn't need to know about all those details.
I think it would be a good idea to encapsulate things a bit more
by changing that to something like

	MR_trace_enabled = saved_trace_enabled;
	MR_init_tracing();

where MR_init_tracing() is a function or macro defined in one of
the MR_trace_*.[hc] files.

> Index: tests/debugger/runtests
> +	/bin/rm *.res

Change that to

	rm *.res

On some systems, `rm' is not in /bin.

I'd like to see a relative diff for any fixes to address the points
raised above.

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