[m-dev.] for review: big step towards a usable debugger, round 3 (part 3 of 3)

[Fergus Henderson]

On 08-Apr-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> --- ws4/runtime/mercury_goto.h	Tue Mar 17 17:38:02 1998
> +++ ws2/runtime/mercury_goto.h	Mon Mar 30 18:56:14 1998
> @@ -35,22 +35,28 @@
>  */
>  
>  #if defined(MR_INSERT_LABELS)
> -  #define	make_label(n, a, l)	make_entry(n, a, l)
> +  #define make_label(n, a, l)		make_entry(n, a, l)
> +  #define make_label_sl(n, a, l)	make_entry_sl(n, a, l)
>  #else
> -  #define	make_label(n, a, l)	/* nothing */
> +  #define make_label(n, a, l)		/* nothing */
> +  #define make_label_sl(n, a, l)	/* nothing */
>  #endif

It would be good to have a comment just above here
explain what the `_sl' versions do.

mercury_trace.c:
> +#define	MR_NAME_LEN		80
> +#define	MR_MAX_SPY_POINTS	20
> +#define	MR_LOG10_MAX_SPY_POINTS	20
> +
> +typedef struct {
> +	bool	enabled;
> +	char	module_name[MR_NAME_LEN];
> +	char	pred_name[MR_NAME_LEN];
> +} MR_spy_point;
> +
> +MR_spy_point	MR_spy_points[MR_MAX_SPY_POINTS];
> +int		MR_next_spy_point = 0;

Hmmm....

It would nicer to dynamically allocate the module name and pred name.
Also I'd like MR_MAX_SPY_POINTS to be higher.

But these can be committed as a seperate change.

> +static bool
> +MR_event_matches_spy_point(const MR_Stack_Layout_Label *layout)
> +{
> +	const	MR_Stack_Layout_Entry	*entry;
> +	int				i;
> +
> +	entry = layout->MR_sll_entry;
> +
> +	for (i = 0; i < MR_next_spy_point; i++) {
> +		if (streq(MR_spy_points[i].pred_name, entry->MR_sle_name)
> +		&& streq(MR_spy_points[i].module_name, entry->MR_sle_def_module)
> +		&& MR_spy_points[i].enabled) {
> +			return TRUE;
> +		}
> +	}

It might be more efficient to test `MR_spy_points[i].enabled' first rather
than last.

> +void
> +MR_trace_report_raw(int fd)
>  {
> +	char	buf[80];	/* that ought to be more than long enough */
> +
>  	if (MR_trace_event_number > 0) {
>  		/*
>  		** This means that the executable was compiled with tracing,
>  		** which implies that the user wants trace info on abort.
>  		*/
>  
> +		sprintf(buf, "Last trace event was event #%d.\n",
>  			MR_trace_event_number);
> +		write(fd, buf, strlen(buf));
>  	}
>  }

I suggest adding the following comment here:

	/*
	** Hopefully sprintf() will be reentrant...
	*/

> @@ -976,7 +1170,9 @@
>  		** fails, are not of interest to the trace analyzer.
>  		*/
>  
> -		if (strncmp(name, "TypeInfo", 8) == 0
> +		if ((strncmp(name, "TypeInfo", 8) == 0)
> +		|| (strncmp(name, "ModuleInfo", 10) == 0)
> +		|| (strncmp(name, "HLDS", 4) == 0)

An XXX comment would be appropriate here.

> -	if (strncmp(name, "TypeInfo", 8) == 0)
> +	if ((strncmp(name, "TypeInfo", 8) == 0)
> +	|| (strncmp(name, "ModuleInfo", 10) == 0)
> +	|| (strncmp(name, "HLDS", 4) == 0))
>  		return;

Ditto.

>  static void
>  MR_trace_help(void)
>  {
> -	fprintf(stderr, "valid commands are:\n"
> +	printf("valid commands are:\n"
>  		"a, EOF:\t\t"
>  		"\tabort the current execution.\n"
> +		"b module pred:\t"
> +		"\tset breakpoint in module:pred.\n"

I think "at" would be better than "in".  Also the documentation
suggests that it only works for predicates, not for functions. 
Hence I suggest the following:

		"b <module> <name>:\t"
		"\tset a breakpoint at the predicate or function named <name>\n"
		"\tdefined in module <module>.\n"

>  		"[<N>] s, [N] CR:"
>  		"\tskip N events, not printing the trace.\n"

You should specify the default value of N.  Also "[<N>] [s]:" might be
a clearer way of specifying that syntax.  So I suggest the following:

  		"[<N>] [s]:"
  		"\tskip N events (default is N = 1),\n"
		"not printing the trace.\n"

There's a few other places where it would be helpful to add
"(default is N = 1)".


That's it for part 3.

All my suggestions are about pretty minor things, so
I don't think I need to see another diff. 

Thanks!

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