[m-dev.] for review: a big step towards the trace-based debugger (part 3 of 3)

[Fergus Henderson]

On 20-Mar-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
>  typedef enum {
> -	MR_CMD_CONT,	/* c: continue to end, not printing the trace	  */
> -	MR_CMD_DUMP,	/* d: continue to end, printing the trace	  */
> -	MR_CMD_NEXT,	/* n: go to the next trace event		  */
> -	MR_CMD_SKIP,	/* s: skip the current call, not printing trace	  */
> -	MR_CMD_JUMP	/* j: jump to end of current call, printing trace */
> +	MR_CMD_GOTO,
> +	MR_CMD_FINISH,
> +	MR_CMD_TO_END
>  } MR_trace_cmd_type;

Some comments explaining the meaning of these enum constants would be
very helpful here.

> -#ifdef MR_USE_DEBUGGER
> +#ifdef MR_USE_EXTERNAL_DEBUGGER

Did you get all occurrences of MR_USE_DEBUGGER,
including the ones in util/mkinit.c?

> +	if (max_r_num + MR_NUM_SPECIAL_REG > MR_MAX_SPECIAL_REG_MR)
> +		max_mr_num = max_r_num + MR_NUM_SPECIAL_REG;
> +	else
> +		max_mr_num = MR_MAX_SPECIAL_REG_MR;

I'd prefer it if you used { and } even for one-line statements.

> +void
> +MR_trace_report(void)
> +{
> +	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.
> +		*/
> +
> +		fprintf(stderr, "Last event was event #%d.\n",
> +			MR_trace_event_number);
> +	}
> +}

I suggest s/Last event/Last trace event/

> +	pseudo_type_info = MR_LIVE_TYPE_GET_VAR_TYPE(var->MR_slv_live_type);
> +	*type_info = (Word) ML_create_type_info(type_params, pseudo_type_info);
> +	*value = MR_trace_lookup_live_lval(var->MR_slv_locn,
> +		&succeeded);

You can unwrap that line.

> @@ -1030,42 +1173,72 @@
>  		** avoid going through call_engine, but for some unknown
>  		** reason, that seemed to cause the Mercury code in the
>  		** browser to clobber part of the C stack.
> +		**
>  		** Probably that was due to a bug which has since been
>  		** fixed, so we should change the code below back again...
> +		**
> +		** call_engine expects the transient registers to be
> +		** in fake_reg, others in their normal homes.
> +		** The code below works by placing r1, r2 and all other
> +		** transient registers both in their normal homes and
> +		** and in fake_reg as well.
>  		*/
> +
> +		MR_trace_enabled = FALSE;
> +		for (i = 0; i < MAX_FAKE_REG; i++) {
> +			fake_reg[i] = MR_saved_regs[i];
> +		}

Shouldn't that be max_mr_rn instead of MAX_FAKE_REG?

> +		restore_registers();
>  		r1 = type_info;
>  		r2 = value;
> +		save_transient_registers(); /* XXX probably redundant now */
>  		call_engine(MR_library_trace_browser);
> +		MR_trace_enabled = TRUE;

What's the XXX there for?
If r1 and/or r2 are transient, then the save_transient_registers()
is not redundant.  I suggest you change the comment to

		save_transient_registers(); /* in case r1 or r2 is transient */

> -		case '1':	
> -			if (sscanf(optarg, "%d", &r1val) != 1)
> -				usage();
> -
> -			break;
> -
> -		case '2':	
> -			if (sscanf(optarg, "%d", &r2val) != 1)
> -				usage();
> -
> -			break;
> -
> -		case '3':	
> -			if (sscanf(optarg, "%d", &r3val) != 1)
> -				usage();
> -
> -			break;
> -

Did you also delete the declaration and definition of the r[1-3]val
global variables?

> New File: scripts/mmd
> ===================================================================
> #!/bin/sh
> case $# in
> 	0)	echo "Usage: mmd <executable> [<arg> ...]"
> 		exit 1;;
> esac
> 
> MERCURY_OPTIONS="$MERCURY_OPTIONS -Di"
> export MERCURY_OPTIONS
> exec "$@"

Please put a comment at the top of this script explaining what it does.

Also as I said earlier, support for a `--help' option and hence
a man page would be a good idea.

OK, that's it for this pass.  When you've addressed the stuff raised in
this review, I'd like to see a diff relative to this current diff.

Cheers,
	Fergus.

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