updated diff for trace-based primitive debugger

[Fergus Henderson]

On 02-Feb-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> +typedef	struct MR_stack_layout_shape_struct {
> +	Word			*MR_stack_layout_shape_type;
> +	Word			MR_stack_layout_shape_inst;
> +} MR_stack_layout_shape;

Any particular reason for the long field names?

We need to use `MR_' to protect against `type' or `inst' being
defined as macros, but I think

	typedef	struct MR_stack_layout_shape_struct {
		Word			*MR_type;
		Word			MR_inst;
	} MR_stack_layout_shape;

should be sufficient.

> +typedef	struct MR_stack_layout_var_struct {
> +	Integer			MR_stack_layout_locn;
> +	MR_stack_layout_shape	*MR_stack_layout_shape;
> +} MR_stack_layout_var;
> +
> +typedef	struct MR_stack_layout_vars_struct {
> +	MR_stack_layout_var	*MR_stack_layout_pairs;
> +	String			*MR_stack_layout_names;
> +	Word			*MR_stack_layout_tvars;
> +} MR_stack_layout_vars;
> +
> +typedef	struct MR_stack_layout_entry_struct {
> +	Code			*MR_stack_layout_code_addr;
> +	MR_Determinism		MR_stack_layout_detism;
> +	Integer			MR_stack_layout_stack_slots;
> +	MR_Live_Lval		MR_stack_layout_succip_locn;
> +	/* the fields from here onwards are present only with procid layouts */
> +	MR_PredFunc		MR_stack_layout_pred_or_func;
> +	String			MR_stack_layout_decl_module;
> +	String			MR_stack_layout_def_module;
> +	String			MR_stack_layout_name;
> +	Integer			MR_stack_layout_arity;
> +	Integer			MR_stack_layout_mode;
> +	/* the fields from here onwards are present only with trace layouts */
> +	Integer			MR_stack_layout_in_arg_count;
> +	MR_stack_layout_vars	MR_stack_layout_in_arg_info;
> +	Integer			MR_stack_layout_out_arg_count;
> +	MR_stack_layout_vars	MR_stack_layout_out_arg_info;
> +} MR_stack_layout_entry;

Ditto.

> +	switch ((int) layout->MR_stack_layout_detism) {
> +		case MR_DETISM_DET:
>  			fprintf(stderr, "DET  ");
>  			break;
>  
> -		case MR_MODEL_SEMI:
> +		case MR_DETISM_SEMI:
>  			fprintf(stderr, "SEMI ");
>  			break;
>  
> -		case MR_MODEL_NON:
> +		case MR_DETISM_NON:
>  			fprintf(stderr, "NON  ");
>  			break;
> +
> +		case MR_DETISM_MULTI:
> +			fprintf(stderr, "MUL  ");
> +			break;
> +
> +		case MR_DETISM_ERRONEOUS:
> +			fprintf(stderr, "ERR  ");
> +			break;
> +
> +		case MR_DETISM_FAILURE:
> +			fprintf(stderr, "FAI  ");
> +			break;
> +
> +		case MR_DETISM_CCNON:
> +			fprintf(stderr, "CCN  ");
> +			break;
> +
> +		case MR_DETISM_CCMULTI:
> +			fprintf(stderr, "CCM  ");
> +			break;

These abbreviations are cryptic.
I suggest increasing the field width from 3 to 9 and
using the same names as in the Mercury language.
(i.e. `det', `semidet', `nondet', `multi',
`failure', `cc_nondet', `cc_multi', and `erroneous').

> +			case 'j':
> +				if (port_is_final(port)) {
> +					fprintf(stderr, "cannot jump from this port\n");
...
> fprintf(stderr, "cannot print from this port\n");
...
> fprintf(stderr, "cannot skip from this port\n");
...
> fprintf(stderr, "are you sure you want to abort? ");

It would be a good idea, I think, to insert "mtrace: " at the start of these
error messages (and any others printed out by runtime/mercury_trace.c).

> +typedef struct {
> +	Word	*saved_succip;
> +	Word	*saved_hp;
> +	Word	*saved_sp;
> +	Word	*saved_curfr;
> +	Word	*saved_maxfr;
> +} MR_ctrl_regs;
> +
> +static	MR_ctrl_regs	saved_ctrl;
> +static	Word		saved_rs[10];
>
> +void	MR_copy_regs_to_save_area(void);
> +void	MR_copy_regs_from_save_area(void);

Why not use just 

	Word 	saved_fake_regs[MAX_FAKE_REG];

with save_registers() and restore_registers() macros to
save/restore the real regs to the fake_reg array and
and memcpy() to save/restore the fake_reg array?

> +			value = saved_rs[locn_num - 1];


That could be done using the virtual_reg() macro or something
analagous.

> +		/* (*MR_library_trace_browser)((Word) type, (Word) value); */
> +		r1 = (Word) type;
> +		r2 = (Word) value;
> +		call_engine(MR_library_trace_browser);

Why use call_engine() rather than the commented out code?

> +int
> +MR_trace_get_cmd(void)
> +{
> +	int	cmd;
> +	int	c;
> +
> +	cmd = getchar();	/* read the trace command */
> +
> +	/* skip the rest of the line */
> +	c = cmd;
> +	while (c != EOF && c != '\n')
> +		c = getchar();
> +
> +	return cmd;
>  }

What happened to the enum that you used for commands?
I think that was a good idea.

Also, single-character command names are nice for interactive use,
but it would be better if the tracer also had long name equivalents.

> --- mercury_wrapper.c	1998/01/06 07:06:06	1.5
> +++ mercury_wrapper.c	1998/02/02 06:34:36
> @@ -127,7 +127,8 @@
>  		/* normally ML_io_init_state (io__init_state/2)*/
>  void	(*MR_library_finalizer)(void);
>  		/* normally ML_io_finalize_state (io__finalize_state/2) */
> -
> +Code	*MR_library_trace_browser;
> +		/* normally mercury__io__print_3_0 (io__print/3) */

Any particular reason why this is exported as a Mercury label
rather than as a C function?

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