updated diff for trace-based primitive debugger

[Zoltan Somogyi]

Fergus wrote:
> 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?

I would like to keep the field names unique, and I would like them to
reflect the struct they are part of (navigating between these structures
is hard enough without being confused about such things). How about picking
an abbreviation for each struct type and using that as a prefix? E.g.
MR_sls_type and MR_sls_inst for the ones above (sls = stack layout shape),
MR_sle_* for the fields of the per-entry struct, MR_sll_* for the per-label
struct, and MR_slv_* for the per-var struct?

> > +		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').

There is only so much space per line. I would like to use as much of the line
as possible for displaying the qualified procedure name, since it can be
quite long. Would DET, SEMI, NON, MUL, ERR, FAIL, CNON and CMUL do?

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

OK.

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

Originally I decided against this because it is slow; MAX_FAKE_REG is 1024.
However, I will adapt this for the time being. The slowness is overshadowed
by the cost of I/O when debugging; we can revisit the cost tradeoff when
we implement the filtering function of the trace analyzer.

> > +		/* (*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?
>
> > --- 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?

If we export io__print as a C function and call it as in the commented out
code above, which is what I did originally, the callback from C to Mercury
clobbers part of the C stack. Although I know where this happens, I don't
know why. Fergus and I should sit down together to find out.

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

This function just gets a char and ignores the rest of the line. Finding
out whether the char is a valid command is the caller's job; there is no
point in duplicating it. Also, some commands the user can give (e.g. "p")
don't have an enum value.

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

There a lot of other things the trace debugger should have, but one step
at a time.

Zoltan.