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

[Zoltan Somogyi]

> stack_layout.m:
> >  :- pred stack_layout__construct_proc_layout(label::in,
> >  	determinism::in, int::in, maybe(int)::in, maybe(label)::in,
> > -	stack_layout_info::in, stack_layout_info::out) is det.
> > +	maybe(int)::in, stack_layout_info::in, stack_layout_info::out) is det.
> 
> Here -- and in lots of other places -- I think it would be clearer
> if you used a typedef called say `maybe_from_full_slot' rather than
> `maybe(int)', particularly if there was a comment next to the
> definition of the type explaining its purpose.

I will keep it maybe(int) for now. I plan to add another slot later for Mark;
I will follow your suggestion then.

> > +		string__append_list([
> > +			FillThreeSlots, "\n",
> > +			"\t\t", RedoLayoutStr, " = (Word) (const Word *) &",
> > +			LayoutAddrStr, ";"
> 
> Why the double cast?  Why not cast straight to `Word'?

> > +	GotoStmt = "\t\tif (MR_jumpaddr != NULL) GOTO(MR_jumpaddr);",
> > +	string__append_list([DeclStmt, SaveStmt, CallStmt, RestoreStmt,
> > +		GotoStmt], TraceStmt),
> 
> Hmm, is the test of `MR_jumpaddr' needed if tracing of REDO events
> is not enabled?

Yes, of course. The test and REDO are not related; the test is to support
the retry command.

> > +** Group (2) fields have a different interpretation if the procedure is
> > +** compiler-generated. You can test for this via the macro
> > +** MR_ENTRY_LAYOUT_COMPILER_GENERATED.
> 
> What's the different interpretation?

It is documented in stack_layout.m.

> > +#define	MR_ENTRY_LAYOUT_COMPILER_GENERATED(entry)		\
> > +		((int) entry->MR_sle_pred_or_func > MR_FUNCTION)
> 
> Hmm, that code is a bit obfuscated -- a comment here would be helpful.
> If special values are being used as sentinels, it would probably
> be a good idea to make those special values part of the enum.

The "special values" are pointers, so they cannot be part of the data type.

> > +void
> > +MR_print_proc_id(FILE *fp, const MR_Stack_Layout_Entry *entry_layout,
> > +	const char *extra)
> > +{
> ...
> > +	fprintf(fp, "%s %s:%s/%ld-%ld (%s)",
> > +		entry_layout->MR_sle_pred_or_func == MR_PREDICATE ?
> > +			"pred" : "func",
> 
> Will this incorrectly print "func" for compiler-generated procedures?

Yes.

> Or is it supposed to be guaranteed that this C function will never be
> called for with an entry_layout whose pred_or_func field is neither
> MR_PREDICATE nor MR_FUNCTION?  If the latter, then that precondition
> should be documented in the header file.

No, it is not guaranteed, only very likely.

I will fix that, but not in this change.

Zoltan.