[m-dev.] for review: new debugger command set (part 1 of 4)

[Fergus Henderson]

> *** ADD TRACE DEPTH HISTOGRAMS ***
> 
> runtime/mercury_conf_param.h:
> 	Document MR_TRACE_HISTOGRAM.
> 
> runtime/mercury_trace_base.[ch]:
> 	Define the data structures for the histogram, and print the histogram
> 	when a traced program exits if MR_TRACE_HISTOGRAM is set.
> 
> trace/mercury_trace.[ch]:
> 	If MR_TRACE_HISTOGRAM is defined, record a count of the number of
> 	events at each depth. This information can help us evaluate space-time
> 	tradeoffs.
...
runtime/mercury_conf_param.h:
> +/*
> +** Experimental options:
> +**
> +** MR_TRACE_HISTOGRAM
> +** Enable this if you want to count the number of execution tracing events
> +** at various call depths.
> +*/

Hmm, would it be better to classify that as a "profiling option" rather than
creating a new category of "experimental options"?

runtime/mercury_trace_base.c:
> +int		*MR_trace_histogram_all = NULL;
> +int		*MR_trace_histogram_exp = NULL;
> +int		MR_trace_histogram_max  = 0;
> +int		MR_trace_histogram_hwm  = 0;

Shouldn't that be inside a #ifdef?

runtime/mercury_trace_base.h:
> +/*
> +** If MR_TRACE_HISTOGRAM is defined, MR_trace maintains two arrays of integers,
> +** MR_trace_histogram_all and MR_trace_histogram_exp, in which the element
> +** with subscript d is incremented when a trace event occurs at depth d.
> +** The intention is that the MR_trace_histogram_all records all events
> +** and is never reset, which means that it records information about a whole
> +** execution of the program. MR_trace_histogram_exp on the other hand can be
> +** zeroed by a command from the debugger at e.g a call port, and examined at
> +** e.g. an exit port, which means that it can record information about the
> +** execution of a call.
> +**
> +** Both arrays are allocated via malloc, and resized on demand. They are
> +** always the same size, and this size is stored in MR_trace_histogram_max.
> +** MR_trace_histogram_hwm stores the high water mark, i.e. the biggest
> +** depth number that has been encountered so far in the execution of the
> +** program.
> +*/
> +
> +#define	MR_INIT_HISTOGRAM_SIZE	128
> +
> +extern	int	*MR_trace_histogram_all;
> +extern	int	*MR_trace_histogram_exp;
> +extern	int	MR_trace_histogram_max;
> +extern	int	MR_trace_histogram_hwm;
> +
> +extern	void	MR_trace_print_histogram(FILE *fp, const char *which,
> +			int *histogram, int max);

Shouldn't that be inside a #ifdef?

> +++ mercury_wrapper.c	1998/09/29 06:39:24
> @@ -138,8 +138,13 @@
> >>> ADD TRACE DEPTH HISTOGRAMS <<<
> +/*
> +** Reserve room for event counts for this many depths initially.
> +*/
>  
> +#define	INIT_HISTOGRAM	128

Hmm, why do you have both INIT_HISTOGRAM and MR_INIT_HISTOGRAM_SIZE?

trace/trace*.c:
> 	} else if (streq(words[0], "histogram_all")) {
> 		if (word_count == 2) {
> 			FILE	*fp;
> 
> 			fp = fopen(words[1], "w");
> 			if (fp == NULL) {
> 				perror(words[1]);

It would be better to report a bit more detail with the error
message, e.g. 

	fprintf(stderr, "mdb: error opening file `%s' for output: %s",
		words[1], strerror(errno));

> 			} else {
> 				MR_trace_print_histogram(fp, "All-inclusive",
> 					MR_trace_histogram_all,
> 					MR_trace_histogram_hwm);
> 				fclose(fp);

You really ought to check the return value of fclose(), so that you
can report things like "disk full" errors.  At very least please
add an "XXX" comment.

> 	} else if (streq(words[0], "histogram_exp")) {
> 		if (word_count == 2) {
> 			FILE	*fp;
> 
> 			if (fp == NULL) {

Here you access `fp' without initializing it.

GNU C should have issued a warning for that one;
please check if there were any other warnings you missed.

> 				perror(words[1]);
> 			} else {
> 				MR_trace_print_histogram(fp, "Experimental",
> 					MR_trace_histogram_exp,
> 					MR_trace_histogram_hwm);
> 				fclose(fp);

The same comments about the calls to perror() and fclose() apply here too.

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