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

[Fergus Henderson]

On 06-Jul-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> Index: runtime/mercury_trace_internal.c
> +#include <unistd.h>

Please document why.
Non-ANSI depencies ought to be minimized.

> +extern	int	optind;

I suggest `#include "mercury_getopt.h"' instead.

> +#define	MR_INIT_SPY_POINTS	10

Please document what that macro is for.

> +/* If you add a usage message, add it to MR_trace_help as well. */
> +static	char	step_usage[] = "step [-NSans] [<number>]";
> +static	char	goto_usage[] = "goto [-NSans] <number>";
> +static	char	finish_usage[] = "finish [-NSans]";
> +static	char	forward_usage[] = "forward [-NSans]";
> +static	char	continue_usage[] = "continue [-NSans]";
> +static	char	restart_usage[] = "restart";
> +static	char	level_usage[] = "level <number>";
> +static	char	up_usage[] = "up <number>";
> +static	char	down_usage[] = "down <number>";
> +static	char	vars_usage[] = "vars";
> +static	char	print_usage[] = "print (<number> | *)";
> +static	char	stack_usage[] = "stack";
> +static	char	stack_regs_usage[] = "stack_regs";
> +static	char	break_usage[] = "break (info | [-PSaei] (<module> <pred> [<arity> [<mode>]] | here))";
> +static	char	enable_usage[] = "enable (<number> | *)";
> +static	char	disable_usage[] = "disable (<number> | *)";
> +static	char	register_usage[] = "register";
> +static	char	module_table_usage[] = "module_table";
> +#ifdef	MR_TRACE_HISTOGRAM
> +static	char	histogram_all_usage[] = "histogram_all";
> +static	char	histogram_exp_usage[] = "histogram_exp";
> +static	char	clear_histogram_usage[] = "clear_histogram";
> +#endif
> +static	char	printlevel_usage[] = "printlevel [none | some | all]";
> +static	char	scroll_usage[] = "scroll [on | off]";
> +static	char	echo_usage[] = "echo [on | off]";
> +static	char	help_usage[] = "help";
> +static	char	quit_usage[] = "quit";

s/char/const char/g

The use of `(', '|' and ')' is confusing, IMHO.
I think it would be better to list the different possibilities
on different lines, e.g.

	Usage:
		enable <number>
		enable *

The complicated syntax for the procedure specifier in the
"break" command is also confusing; I think it would be better
to describe it as taking a <procedure specifier> and then
describe what the syntax for that is, e.g. something like

	Usage:
		break <procedure specifier>
		break here
		break info
	where
		a <procedure specifier> is of the form
			<module> <name> [<arity> [<mode>]]
		<module> is the module name
		<name> is the predicate or function name
		<arity> is the number of arguments
		<mode> is the mode number (0 for the first mode,
			1 for the second mode, etc.)

with the stuff after `where' not appearing in the general summary.

Speaking of which, a `help <command>' command would be helpful! ;-)

> +	/*
> +	** These globals can be overwritten when we call Mercury code,
> +	** such as the term browser. We therefore save and restore them
> +	** across calls to MR_trace_debug_cmd. However, we store the
> +	** saved values in global instead of local variables, to allow them
> +	** to be modified by MR_trace_restart().
> +	*/
> +
> +	MR_trace_saved_call_seqno = MR_trace_call_seqno;
> +	MR_trace_saved_call_depth = MR_trace_call_depth;
> +	MR_trace_saved_event_number = MR_trace_event_number;

These variables (both those on LHS and RHS) should all be thread-local,
in case you're running two copies of the debugger to debug two concurrent
threads.

The way to make them thread-local is to put them in the MercuryEngine
structure.

It may be best to leave worrying about that to a seperate change,
but please at least put an XXX comment for it.

> +static void
> +MR_trace_internal_ensure_init(void)
> +{
> +	static	bool	done = FALSE;
> +
> +	if (! done) {
> +		printf("Melbourne Mercury Debugger\n");
> +		done = TRUE;
> +	}
> +}

Let me bring your attention to 2(c) in the GPL:

 |     c) If the modified program normally reads commands interactively
 |     when run, you must cause it, when started running for such
 |     interactive use in the most ordinary way, to print or display an
 |     announcement including an appropriate copyright notice and a
 |     notice that there is no warranty (or else, saying that you provide
 |     a warranty) and that users may redistribute the program under
 |     these conditions, and telling the user how to view a copy of this
 |     License.  (Exception: if the Program itself is interactive but
 |     does not normally print such an announcement, your work based on
 |     the Program is not required to print an announcement.)

So if you're going to print any announcement, then you need to
include something similar to that printed by gdb:

 | GDB is free software and you are welcome to distribute copies of it
 |  under certain conditions; type "show copying" to see the conditions.
 | There is absolutely no warranty for GDB; type "show warranty" for details.
 | GDB 4.14 (alpha-dec-osf3.2), Copyright 1995 Free Software Foundation, Inc.

> +	} else if (streq(words[0], "break")) {

This is a very long function; it might be better to
split out the code for each command into seperate
subroutines.  The code for handling "break", at least,
is quite long and complex and should be a seperate
subroutine.  Also, there's a lot of duplicate code in the
handling of that case, which should be avoided.

Instead of having a seperate function for each possibility,
i.e. whether the module, arity, or proc number were specified,
you should have a single function which gets passed a
MR_Proc_Specifier struct, whose fields can have special
values that indicate "any", e.g. a NULL module name means
any module matches.

>  static void
> +MR_trace_restart(const MR_Stack_Layout_Label *this_label, int seqno, int depth)
> +{
> +	args = checked_malloc(arg_max * sizeof(Word));

Storage allocated by checked_malloc() is NOT scanned by the conservative
garbage collector.  Either you should add a comment indicating your
reasoning as to why this should use checked_malloc(), or you should use
make_array() [which calls newmem(), which calls GC_MALLOC(),
which calls GC_malloc()] instead.

You should check all your other calls to checked_malloc() too.

Also it would be useful to define a macro like make_array() which
calls checked_malloc() instead of newmem(), and to use it rather
than calling checked_malloc() directly.

> +static Word
> +MR_trace_find_input_arg(const MR_Stack_Layout_Label *label,
> +	const char *name, bool *succeeded)
> +{
...
> +				TRUE, MR_saved_sp(MR_saved_regs), MR_saved_curfr(MR_saved_regs), succeeded);

Please wrap that line.

> +bool
> +MR_event_matches_spy_point(const MR_Stack_Layout_Label *layout,
> +	MR_Trace_Port port, MR_Spy_Action *action_ptr)
> +{
...
> +	hi = MR_spied_procs_next-1;

s/-/ - /

> +	found = FALSE;
> +	while (lo <= hi) {
> +		mid = (lo + hi) / 2;
> +		if (MR_spied_procs[mid].spy_proc == entry) {
> +			found = TRUE;
> +			break;
> +		} else if (MR_spied_procs[mid].spy_proc < entry) {
> +			lo = mid + 1;
> +		} else {
> +			hi = mid - 1;
> +		}
> +	}
> +
> +	if (! found) {
> +		return FALSE;
> +	}
> +
> +	enabled = FALSE;
> +	action = MR_SPY_PRINT;
> +	for (point = MR_spied_procs[mid].spy_points; point != NULL;
> +			point = point->spy_next) {
> +		if (! point->spy_enabled) {
> +			continue;
> +		}
> +
> +		switch (point->spy_when) {
> +
> +			case MR_SPY_ALL:
> +				enabled = TRUE;
> +				action = max(action, point->spy_action);

You need a `break;' here, I think.

> +			case MR_SPY_ENTRY:
> +				if (MR_port_is_entry(port)) {
> +					enabled = TRUE;
> +					action = max(action, point->spy_action);
> +				} else {
> +					continue;
> +				}

Ditto.

> +			case MR_SPY_INTERFACE:
> +				if (MR_port_is_interface(port)) {
> +					enabled = TRUE;
> +					action = max(action, point->spy_action);
> +				} else {
> +					continue;
> +				}

Ditto.

> +			case MR_SPY_SPECIFIC:
> +				if (layout == point->spy_label) {
> +					enabled = TRUE;
> +					action = max(action, point->spy_action);
> +				} else {
> +					continue;
> +				}

Ditto.

> +MR_Spy_Point *
> +MR_add_spy_point(MR_Spy_When when, MR_Spy_Action action,
> +	const MR_Stack_Layout_Entry *entry, const MR_Stack_Layout_Label *label,
> +	const char *path)
> +{
> +	MR_Spy_Point	*point;
> +	int		lo;
> +	int		hi;
> +	int		mid;
> +	bool		found;
> +
> +	lo = 0;
> +	hi = MR_spied_procs_next-1;
> +	found = FALSE;
> +	while (lo <= hi) {
> +		mid = (lo + hi) / 2;
> +		if (MR_spied_procs[mid].spy_proc == entry) {
> +			found = TRUE;
> +			break;
> +		} else if (MR_spied_procs[mid].spy_proc < entry) {
> +			lo = mid + 1;
> +		} else {
> +			hi = mid - 1;
> +		}
> +	}
> +
> +	if (! found) {

I detect some duplicate code, that ought to be in a subroutine.

> +		/* make sure the MR_spied_procs array is big enough */
> +		if (MR_spied_procs_next >= MR_spied_procs_max) {
> +			if (MR_spied_procs_max == 0) {
> +				MR_spied_procs_max = INIT_SPY_TABLE_SIZE;
> +				MR_spied_procs = checked_malloc(
> +							MR_spied_procs_max *
> +							sizeof(MR_Spied_Proc));
> +			} else {
> +				MR_spied_procs_max *= 2;
> +				MR_spied_procs = checked_realloc(
> +							MR_spied_procs,
> +							MR_spied_procs_max *
> +							sizeof(MR_Spied_Proc));
> +			}
> +		}

That code fragment too is something I have seen many occurrences of
(although the variable names are always different).
You should use a macro.

> +/*
> +** The only reason why MR_need_move is function is that

s/is function/is a function/

> +** gcc gets an internal error ("fixed or forbidden register was spilled")
> +** if it isn't a function.

Please name the version number and architecture for which this occurs.

(Did you send a bug report to the gcc developers?)

> +static MR_Module_Info *
> +MR_search_module_info(const char *name)
> +{
> +	int	lo;
> +	int	hi;
> +	int	mid;
> +	int	diff;
> +
> +	lo = 0;
> +	hi = MR_module_info_next-1;
> +	while (lo <= hi) {
> +		mid = (lo + hi) / 2;
> +		diff = strcmp(MR_module_infos[mid].MR_module_name, name);
> +		if (diff == 0) {
> +			return &MR_module_infos[mid];
> +		} else if (diff < 0) {
> +			lo = mid + 1;
> +		} else {
> +			hi = mid - 1;
> +		}
> +	}

More duplicate code.

> +static MR_Module_Info *
> +MR_insert_module_info(const char *name)
> +{
> +	int	i;
> +
> +	/* make sure the MR_module_infos array is big enough */
> +	if (MR_module_info_next >= MR_module_info_max) {
> +		if (MR_module_info_max == 0) {
> +			MR_module_info_max = INIT_MODULE_TABLE_SIZE;
> +			MR_module_infos = checked_malloc(
> +						MR_module_info_max *
> +						sizeof(MR_Module_Info));
> +		} else {
> +			MR_module_info_max *= 2;
> +			MR_module_infos = checked_realloc(
> +						MR_module_infos,
> +						MR_module_info_max *
> +						sizeof(MR_Module_Info));
> +		}
> +	}

Ditto.

> Index: runtime/mercury_trace_tables.h
> +extern	void		MR_register_all_modules_and_procs(FILE *fp);
> +extern	void		MR_dump_module_tables(FILE *fp);
> +
> +extern	const MR_Stack_Layout_Entry
> +			*MR_search_for_module_proc(const char *module_name,
> +				const char *pred_name, bool *unique);
> +extern	const MR_Stack_Layout_Entry
> +			*MR_search_for_module_proc_arity(const char
> +				*module_name, const char *pred_name,
> +				int arity, bool *unique);
> +extern	const MR_Stack_Layout_Entry
> +			*MR_search_for_module_proc_arity_mode(const char
> +				*module_name, const char *pred_name,
> +				int arity, int mode, bool *unique);
> +extern	const MR_Stack_Layout_Entry
> +			*MR_search_for_module_proc_arity_mode_pf(const char
> +				*module_name, const char *pred_name,
> +				int arity, int mode, MR_PredFunc predfunc,
> +				bool *unique);
> +extern	void		MR_process_all_module_proc(
> +				void f(const MR_Stack_Layout_Entry *),
> +				const char *module_name, const char *pred_name);
> +extern	void		MR_process_all_module_proc_arity(
> +				void f(const MR_Stack_Layout_Entry *),
> +				const char *module_name, const char *pred_name,
> +				int arity);
> +extern	void		MR_process_all_module_proc_arity_mode(
> +				void f(const MR_Stack_Layout_Entry *),
> +				const char *module_name, const char *pred_name,
> +				int arity, int mode);
> +extern	void		MR_process_all_module_proc_arity_mode_pf(
> +				void f(const MR_Stack_Layout_Entry *),
> +				const char *module_name, const char *pred_name,
> +				int arity, int mode, MR_PredFunc predfunc);

Please document these functions.

> Index: runtime/mercury_trace_util.c
...
> +int
> +MR_trace_get_register_number(MR_Live_Lval locn)
> +{
> +	int	locn_num;
> +	Word	value;
> +
> +	if (MR_LIVE_LVAL_TYPE(locn) == MR_LVAL_TYPE_R) {
> +		return MR_LIVE_LVAL_NUMBER(locn);
> +	} else {
> +		return -1;
> +	}
> +}

Unused variables `value' and `locn_num'.

...
> Index: runtime/mercury_trace_util.h
> +extern	int	MR_trace_get_register_number(MR_Live_Lval locn);

Please document that function; make sure you explain the role of the
magic number -1.

(The other functions declared in that header ought to be documented too...
though not necessarily as part of this change.)

> RCS file: /home/mercury1/repository/mercury/runtime/mercury_wrapper.c,v
> retrieving revision 1.15
> diff -u -u -r1.15 mercury_wrapper.c
> --- mercury_wrapper.c	1998/07/03 02:35:41	1.15
> +++ mercury_wrapper.c	1998/07/03 12:18:52
> @@ -710,9 +710,11 @@
>    #ifndef CONSERVATIVE_GC
>  	heap_zone->max      = heap_zone->min;
>    #endif
> +#if 0
>  	detstack_zone->max  = detstack_zone->min;
>  	nondetstack_zone->max = nondetstack_zone->min;
> +#endif
>  #endif
>  
>  	time_at_start = MR_get_user_cpu_miliseconds();
>  	time_at_last_stat = time_at_start;
> @@ -747,10 +749,12 @@
>  		printf("max heap used:      %6ld words\n",
>  			(long) (heap_zone->max - heap_zone->min));
>    #endif
> +  #if 0
>  		printf("max detstack used:  %6ld words\n",
>  			(long)(detstack_zone->max - detstack_zone->min));
>  		printf("max nondstack used: %6ld words\n",
>  			(long) (nondetstack_zone->max - nondetstack_zone->min));
> +  #endif
>  	}
>  #endif

This change was not mentioned in the log message.
I don't understand it.

--------------------

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.