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

[Fergus Henderson]

Zoltan Somogyi, you wrote:
> 
> Some commands take a number as their first parameter. For such commands,
> users can type "number command" as well as "command number".

I suggest s/"number command"/"<number> <command>"/
and similarly for the other order.

> FORWARD MOVEMENT COMMANDS
> 
> step [-NSans] [<num>]

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

> 	If this command is given at event <cur>, continues execution until
> 	event <cur> + <num>. The default value of <num> is 1.

I suggest you add

	Steps forward <num> events.

at the start.

> 	The options -n or --none, -s or --some, -a or --all specify
> 	the print level to use for the duration of the command,
> 	while the options -S or --strict and -N or --nostrict specify
> 	the strictness of the command.

I suggest you put the option names in single quotes `like this'.
(Likewise for all the other commands.)

In fact this document should probably be rewritten in Texinfo,
and added to the Mercury user's guide,
in which case you should put the option names in `@samp{...}'.
I think the documentation should preferably be added in the
same commit as these changes.

> goto [-NSans] <num>
> 	Continues execution until event <num> provided that the number
> 	of the current event is smaller than <num>. Otherwise it reports
> 	an error.

I suggest you word that as

 	Continues execution until the program reaches event number <num>.
 	If the current event number is larger than <num>, it reports
 	an error.

> restart
> 	Restarts execution at the call port of the call corresponding to the
> 	current event. Reports an error if the current event does not refer
> 	to an exit or fail port.

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

> 	The command will report an error unless the values of all the input
> 	arguments are available at the current port. (The compiler will keep
> 	the values of the input arguments of traced predicates as long as
> 	possible, but it cannot keep them beyond the point where they are
> 	destructively updated.) The exception is values of type io__state;
> 	the debugger can perform a restart if the only missing value is of
> 	type io__state (there can be only one io__state at any given time).
> 	(THE EXCEPTION IS NOT YET IMPLEMENTED.)

`io__state' should be in quotes here.

> 	a restart has the following effects:
> 
> 	-	Any input and/or output actions in the repeated code
> 		will be repeated.
> 
> 	-	Any file close actions in the repeated code for which the
> 		corresponding file open action is not also in the repeated
> 		code may cause later I/O actions referring to the file to fail.
> 
> 	-	Any file open actions in the repeated code for which the
> 		corresponding file close action is not also in the repeated
> 		code may cause later file open actions to fail due to file
> 		descriptor leak.

s/a restart/A restart/

These effects sound a bit scary; I think that before listing them,
you should first point out that restart works fine for code that does not
do I/O.

> 	The restriction to exit and fail ports is for implementation reasons:
> 	only at these ports does the debugger have have enough information
> 	to figure out how to reset the stacks.
> 
> 	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?

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.

If you mean the latter, it would probably be worth explicitly mentioning
the "finish" command.

> DEBUGGER STATE MANIPULATION COMMANDS
> 
> break [-PSaei] <module name> <predicate name> [<arity> [<mode> [<predfunc>]]]
> 	Puts a break point on the specified procedure.

s/procedure/procedure(s)/

I think the syntax should be

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

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

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

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

> ---------------------------------------------------------------------------
> 
> Estimated hours taken: 32
> 
> Move the debugger command set a large step towards the proposal previously
> circulated.
> 
> 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.

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

> runtime/mercury_stack_layout.h:
> 	Add the field holding the slot number of the call event number
> 	to MR_Stack_Layout_Entry, and add a macro to make it more convenient
> 	to disentangle the real variablee name from the number:name pair.

s/ee/e/

> runtime/mercury_trace.[ch]:
> 	If MR_TRACE_HISTOGRAM is defined,

You should document MR_TRACE_HISTOGRAM in runtime/mercury_conf_param.h.

> retrieving revision 1.225
> diff -u -u -r1.225 llds.m
> --- llds.m	1998/06/09 02:13:21	1.225
> +++ llds.m	1998/07/05 05:43:34
> @@ -416,15 +412,15 @@
>  	% live_value_type describes the different sorts of data that
>  	% can be considered live.
>  :- type live_value_type 
> -	--->	succip		% a stored succip
> -	;	curfr		% a stored curfr
> -	;	maxfr		% a stored maxfr
> -	;	redoip
> -	;	hp
> -	;	var(type, inst)	% a variable
> -	;	unwanted.	% something we don't need, or used as
> -				% a placeholder for non-accurate gc.
> -	
> +	--->	succip				% a stored succip
> +	;	curfr				% a stored curfr
> +	;	maxfr				% a stored maxfr
> +	;	redoip				% a stored redoip
> +	;	hp				% a stored head pointer
> +	;	var(var, string, type, inst)	% a variable

What does the string hold?

> --- stack_layout.m	1998/06/18 06:06:49	1.14
> +++ stack_layout.m	1998/07/06 10:18:41
> @@ -66,15 +66,21 @@
>  % The meanings of the fields in both forms are the same as in procedure labels.
>  %
>  % If the option trace_stack_layout is set, i.e. if we are doing execution
> -% tracing, the table will also include one extra field:
> +% tracing, the table will also include two extra fields:
>  %
>  %	call trace info		(Word *) - pointer to label stack layout
> +%	call event number	(Integer) - stack slot number storing the
> +%					event number of the call event
>  %
> -% This will point to the per-label layout info for the label associated
> +% The first will point to the per-label layout info for the label associated
>  % with the call event at the entry to the procedure. The purpose of this
>  % information is to allow the runtime debugger to find out which variables
>  % are where on entry, so it can reexecute the procedure if asked to do so
>  % and if the values of the required variables are still available.
> +% The second field allows the debugger to find the event number of the
> +% call event, so that if the user asks for the call to be restarted,
> +% the restarted execution will use the same event numbers as the original
> +% execution.

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.

> +:- pred stack_layout__get_name_from_live_value_type(live_value_type::in,
> +	string::out) is det.
> +
> +stack_layout__get_name_from_live_value_type(LiveType, Name) :-
> +	( LiveType = var(_, NamePrime, _, _) ->
> +		Name = NamePrime
> +	;
> +		Name = ""
> +	).

Please s/NamePrime/Name0/

> +stack_layout__construct_liveval_name(var_info(_, VarInfo), MaybeRval) :-
> +	(
> +		VarInfo = var(Var, Name, _, _),
> +		Name \= ""
> +	->
> +		term__var_to_int(Var, Int),
> +		string__int_to_string(Int, IntStr),
> +		string__append_list([IntStr, ":", Name], NumberedName),
> +		Rval = const(string_const(NumberedName))

A comment here, please.

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

> +void
> +MR_print_proc_id_to_stdout(const MR_Stack_Layout_Entry *entry_layout)
> +{
> +	MR_print_proc_id(stdout, entry_layout, NULL);
> +}

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.

> Index: runtime/mercury_trace.c
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/runtime/mercury_trace.c,v
> retrieving revision 1.14
> diff -u -u -r1.14 mercury_trace.c
> --- mercury_trace.c	1998/07/03 02:35:33	1.14
> +++ mercury_trace.c	1998/07/06 10:01:00
> @@ -25,21 +25,28 @@
...
> +Code				*MR_trace_goto = NULL;
> +int				MR_trace_goto_reg_count = 0;

The fact that this is a global variable may cause problems for debugging
multithreaded code, I think.

> -static	MR_trace_cmd_info	MR_trace_ctrl = { MR_CMD_GOTO, 0, 0, FALSE };
> +static	MR_Trace_Cmd_Info	MR_trace_ctrl = { MR_CMD_GOTO, 0, 0,

>  	switch (MR_trace_ctrl.MR_trace_cmd) {
>  		case MR_CMD_FINISH:
>  			if (MR_trace_ctrl.MR_trace_stop_seqno == seqno
> -			&& MR_port_is_final(port)) {
> +					&& MR_port_is_final(port)) {
>  				MR_trace_event(&MR_trace_ctrl, layout,

Our layout convention for if-then-elses like this whose condition doesn't fit
on a single line is for the `{' to go on a line of its own.

> +		default:
> +			fatal_error("invalid cmd in MR_trace");
> +			return;

The return is unreachable, so you might as well delete it.

> +		if (! match) {
> +			if (MR_trace_ctrl.MR_trace_print_level ==
> +					MR_PRINT_LEVEL_ALL) {

{ should be on its own line here too.

>  static void
> -MR_trace_event(MR_trace_cmd_info *cmd,
> -	const MR_Stack_Layout_Label *layout, MR_trace_port port,
> +MR_trace_event(MR_Trace_Cmd_Info *cmd,
> +	const MR_Stack_Layout_Label *layout, MR_Trace_Port port,
>  	Unsigned seqno, Unsigned depth, const char *path, int max_r_num)
>  {
>  	int	max_mr_num;
> @@ -126,6 +199,10 @@
>  	}
>  
>  	MR_copy_regs_to_saved_regs(max_mr_num);
> +
> +	MR_trace_goto = NULL;
> +	MR_trace_goto_reg_count = max_mr_num;
> +
>  #ifdef MR_USE_EXTERNAL_DEBUGGER
>  	if (MR_trace_debugger == MR_TRACE_EXTERNAL) {
>  		MR_trace_event_external(layout, port, seqno, depth, path);
> @@ -140,18 +217,19 @@
>  
>  	MR_trace_event_internal(cmd, layout, port, seqno, depth, path);
>  #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?

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.

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

Document these, please.

> +++ mercury_trace_base.h	1998/07/05 04:02:53
> @@ -71,4 +71,16 @@
>  extern	void	MR_trace_report(FILE *fp);
>  extern	void	MR_trace_report_raw(int fd);
>  
> +#ifdef	MR_TRACE_HISTOGRAM
> +#define	MR_INIT_HISTOGRAM_SIZE	128
> +
> +extern	int	*MR_trace_histogram_all;
> +extern	int	*MR_trace_histogram_exp;
> +extern	int	MR_trace_histogram_max;
> +extern	int	MR_trace_histogram_hwm;

Document these, please.

> +extern	void	MR_trace_print_histogram(FILE *fp, const char *which,
> +			int *histogram, int max);
> +#endif	/* MR_TRACE_HISTOGRAM */
> +
>  #endif /* MERCURY_TRACE_PERMANENT_H */

The wrapper guard on that header file should be changed to match the
file name.


I'd like to see a relative diff next time.

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.