[m-dev.] for review: new debugger command set, part 1

[Zoltan Somogyi]

> In fact this document should probably be rewritten in Texinfo,
> and added to the Mercury user's guide,

I was always aiming for that, but it easier to do drafts in text,
and convert to texinfo format when it has stabilised.

> > step [-NSans] [<num>]
>
> I suggest you write `[<options>]' rather than `[-NSans]'.
> (Likewise for all the other commands.)

Once we have long options, we won't have room for listing them all,
but until then it would be better to list them all. Are you sure?

> > restart
> 
> I'm not sure this is a good name for this command.
> What will we call the command to restart the whole program?

run?

I am not sure what would be a better name (redo and retry are also bad).
How about "again"? Any other suggestions?

> > 	This limitation can be worked around by letting the program continue
> > 	*forward* execution until it reaches a port on the relevant predicate
> > 	that is outside the commit, and then performing a restart. This
> > 	may require a noticeable amount of time, and may result in the
> > 	execution of I/O and/or other side-effects.
> 
> What do you mean by "the commit" -- which commit?

Sorry, that was a holdover from old text.

> Are you suggesting that the implementation could work around it,
> or are you advising the user on how to work around it?
> This is not clear.

As far as yesterday's changes go, the latter. By the end of the week,
the former. I'll change it for now.

> > ... <module name> <predicate name> [<arity> [<mode> [<predfunc>]]]
> 
> 	[<predfunc>] [<module name>:]<predicate name>[/<arity>] [mode <mode>]

or the format which is used by the debugger when it prints procedure ids:

[<predfunc>] [<module name>:]<predicate name>[/<arity>][-mode <mode>]

In any scheme involving modulename:predname, we should also accept __
instead of :.

Any preferences from other Mercuryites?

> > MISCELLANEOUS COMMANDS
> > 
> > printlevel [<printlevel>]
> > 	If the argument is present, sets the default print level
> > 	to the named level. If it is absent, reports the current default
> > 	print level.
> 
> What is the print level used for?
> A brief explanation and/or a cross-reference would be helpful here.

It was explained at the top, so a cross-reference would be called for here.

> > scroll [on | off]
> > 	If the argument is present, turn user control over the scrolling
> > 	of sequences of event reports on or off. If it is absent, reports
> > 	whether user scroll control is enabled or not.
> > 
> > 	When user scroll control is enabled, every 20th report of an event
> > 	will be followed by a --more-- prompt. The user may type an empty
> > 	line, which allows the debugger to continue to print the next batch
> > 	of event reports. By typing a line that starts with "a", "s" or "n",
> > 	the user can override the print level of the current command, setting
> > 	it to "all", "some" or "none" respectively. By typing a line that
> > 	starts with "q", the user can abort the current command and get back
> > 	control at the next event.
> 
> You shouldn't hard-code the "20".

Well, what we want is to automatically get the window size from the operating
system, but that is future work. I didn't think there would be *too* much point
in providing a user command for setting it, but providing one would be trivial.

> > quit
> > 	Quits the debugger and aborts the execution of the program.
> > 	Asks confirmation first, which is given by any answer starting
> > 	with "y" or EOF.
> > 
> > 	EOF on the debugger's input is considered a quit command.
> 
> There should be an unconditional quit command which does not ask
> confirmation.

How about "quit -y"?

> > compiler/trace.m:
> > 	After every call to MR_trace(), emit code that checks whether it
> > 	should jump away, and if yes, performs the jump. This is used to
> > 	implement restart. (The debugger cannot execute the jump itself
> > 	because it is in the wrong C stack frame.)
> 
> Ouch -- this is going to be expensive.

Yes, but for the time being I can't think of a way to implement restart
that avoids this cost, and I think restart is sufficiently important that
the cost is worthwhile.

(Actually, while your suggestion below does not avoid this cost,
it reduces it a bit.)

> > compiler/stack_layout.m:
> > 	Include the variable number as a prefix in variable names. For
> > 	example, a variable whose name is X and number is 5 is recorded
> > 	in the name array as "5:X".
> 
> Hmm, it might be cleaner to make the variable number a separate field.

I thought about that, but almost all numbers are one or two digits, so
they cost only 2 or 3 bytes, and event that only when the string isn't
common with other layout slots. Any separate field would have a cost
significantly higher than this.

Only the debugger needs to know about this convention; gc ignores the names
altogether.

> > +	;	var(var, string, type, inst)	% a variable
> 
> What does the string hold?

The var's name. It is there explicitly because when we want to use this info,
the varset is not available. I have documented this.

> Any particular reason why you don't just always use say stack slot 2
> for the call event number?  If so, you should document that reason here.

Because by the time trace__init is called, the stack slots of the variables
have already been allocated, so we can't depend on getting a slot with a
given number.

However, I will change the code generator so that if we are doing tracing,
slots 1, 2 and 3 are always reserved for the event number, call number
and call depth.

This allows us to avoid introducing the new field in proc layouts.
It also allows us to avoid passing the call number and depth to MR_trace(),
since MR_trace can get it via the proc_layout. However, the extraction
process is complicated, so I won't do it in this change.

> > +stack_layout__get_name_from_live_value_type(LiveType, Name) :-
> > +	( LiveType = var(_, NamePrime, _, _) ->
> > +		Name = NamePrime
> > +	;
> > +		Name = ""
> > +	).
> 
> Please s/NamePrime/Name0/

I don't see that in the coding guidelines, and the Prime convention is
used in many other places. The 0 suffix is not really appropriate, because
this is not a different version of Name.

> > Index: runtime/mercury_misc.c
> > Index: runtime/mercury_overflow.h
> 
> I think I already saw those changes in a previously posted diff.
> Did you intend to include them in this diff?

No, I didn't; I have committed them as part of the other diff.

> What we really ought to do is to define
> 
> 	FILE *MR_debugger_in;	/* normally stdin */
> 	FILE *MR_debugger_out;	/* normally stdout */
> 
> and initialize them to stdout and stdin in the initialization code.
> This will make it easier to later support putting the debugger output
> and the program output in different windows.
> 
> Now that suggestion need not be part of this change, but
> I think it may not be a good idea to embed "stdout" in the function
> name if this is going to change later.

I agree. I'll change the function name to MR_print_proc_id_for_debugger()
for now.

> >  #endif
> > -	MR_copy_saved_regs_to_regs(max_mr_num);
> > +	MR_copy_saved_regs_to_regs(MR_trace_goto_reg_count);
> >  }
> 
> Hmm... why do you need MR_trace_goto_reg_count?

Because it is possible that at an exit port, only (say) 2 regs are live, but
on entry, (say) 6 are. (While all the input args must survive to the exit,
they may be in stack slots.) So normally, MR_copy_saved_regs_to_regs would
only copy registers up to r2 from the saved reg array to the real registers
(and to fake_reg).

> One thing that would make the check after each call to MR_trace()
> a little more efficient and which would avoid some of the problems
> with multi-threading would be to have MR_trace() return the
> MR_trace_goto value, rather than using a global variable.

That's a good idea. It would also solve the immediate problem with
multithreading. However, this means that the code we have to generate for
events now looks like this:

	Code *jumpaddr;

	save_transient_registers();
	jumpaddr = MR_trace(...);
	restore_transient_registers();
	if (jumpaddr != NULL) GOTO(jumpaddr);

> > Index: runtime/mercury_trace.h
> ...
> > +extern	Code	*MR_trace_goto;
> > +extern	int	MR_trace_goto_reg_count;
> 
> Document these, please.

Actually, both can now be arguments returned to MR_trace (for the address)
and to MR_trace_internal (for the count) by reference.

Zoltan.