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

[Fergus Henderson]

mercury_trace.c:
> /* XXX We should consider whether the following should be thread local. */
> static	MR_Spy_Point    	**MR_spy_points;
> static	int			MR_spy_point_next = 0;
> static	int			MR_spy_point_max  = 0;
> 
> static	MR_Trace_Print_Level	MR_default_print_level = MR_PRINT_LEVEL_SOME;
> 
> static	bool			MR_scroll_control = FALSE;
> static	int			MR_scroll_next = 0;
> static	int			MR_scroll_limit = 24;
> static	bool			MR_echo_commands = FALSE;
> 
> static	int			MR_trace_saved_call_seqno;
> static	int			MR_trace_saved_call_depth;
> static	int			MR_trace_saved_event_number;
>
> typedef struct MR_Line_Struct {
> 	char			*MR_line_contents;
> 	struct MR_Line_Struct	*MR_line_next;
> } MR_Line;
> 
> static	MR_Line			*MR_line_head = NULL;
> static	MR_Line			*MR_line_tail = NULL;

It's not clear what is meant by "the following" in that comment --
does it just refer to the MR_spy_* variables, or is it all of the
variables above?

It would be a good idea to add comments explaining the meaning of
those static variables.

> Code *
> MR_trace_event_internal(MR_Trace_Cmd_Info *cmd, bool interactive,
...
> 	/*
> 	** 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_retry().
> 	*/
> 
> 	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;

It would perhaps be safer to save their values in a local struct,
using a global pointer to point to the local struct;
if you save the global pointer's old value on entry
and restore it on exit, then the code would work even
if MR_trace_event_internal() ends up being called recursively.

(Alternatively, you could pass a pointer to this struct
all the way down to MR_trace_retry(), rather than making
the pointer to this struct a global or static variable.)

> static const char MR_trace_banner[] =
> "Melbourne Mercury Debugger, mdb version 0.8.\n\

The version number should not be hard-coded.
Use the MR_VERSION macro instead.

> Copyright 1998 University of Melbourne, Australia.\n\

s/University/The University/

> static	bool	MR_trace_internal_initialized = FALSE;
> 
> static void
> MR_trace_internal_ensure_init(void)
> {
> 	if (! MR_trace_internal_initialized) {

Shouldn't that variable be local to this function?

> 		MR_trace_internal_init_from_env("MERCURY_DEBUGGER_INIT") ||
> 		MR_trace_internal_init_from_file(MDBRC_FILENAME) ||
> 		MR_trace_internal_init_from_env_dir("HOME", MDBRC_FILENAME) ||
> 		MR_trace_internal_init_from_env("DEFAULT_MERCURY_DEBUGGER_INIT");

A comment here would be helpful.

The last line doesn't fit in 80 columns.

> 
> static bool
> MR_trace_internal_init_from_env(const char *env_var)
> {
> 	char	*init;
> 
> 	init = getenv(env_var);
> 	if (init != NULL) {
> 		MR_trace_source(init);
> 		return TRUE;

Hmm, what if MR_trace_source fails, e.g. because the file doesn't exist?
Shouldn't those two lines be `return MR_trace_source(init);'?

> static bool
> MR_trace_internal_init_from_env_dir(const char *env_var, const char *filename)
> {
> 	char	*env;
> 	char	*buf;
> 	int	len;
> 	FILE	*fp;
> 
> 	env = getenv(env_var);
> 	if (env == NULL) {
> 		return FALSE;
> 	}
> 
> 	buf = checked_malloc(strlen(env) + strlen(filename) + 2);
> 	(void) strcpy(buf, env);
> 	(void) strcat(buf, "/");
> 	(void) strcat(buf, filename);

The use of "/" as a directory separator is system-specific.
On some systems, it should be "\\" or ":" instead.
You should define a macro called say MR_DIRECTORY_SEPARATOR
in mercury_conf.h and use that.

> static MR_Next
> MR_trace_debug_cmd(char *line, MR_Trace_Cmd_Info *cmd,
> 	const MR_Stack_Layout_Label *layout, Word *saved_regs,
> 	MR_Trace_Port port, int seqno, int depth, const char *path,
> 	int *ancestor_level, int *max_mr_num, Code **jumpaddr)
> {
> 	char		**words;
> 	char		**orig_words;
> 	int		word_max;
> 	int		word_count;
> 	const char	*alias_key;
> 	char		**alias_words;
> 	int		alias_word_count;
> 	int		alias_copy_start;
> 	const char	*problem;
> 	char		*semicolon;
> 	int		i;
> 	int		n;

This function is very long and has a lot of variables.
It would be nice if you could split it into smaller functions.

> 	if (MR_echo_commands) {
> 		fputs(line, stdout);
> 		putc('\n', stdout);
> 	}
> 
> 	if ((semicolon = strchr(line, ';')) != NULL) {
> 		/*
> 		** The line contains at least two commands.
> 		** Execute only the first command now; put the others
> 		** back in the input to be processed later.
> 		*/
> 
> 		*semicolon = '\0';
> 		MR_insert_line_at_head(MR_copy_string(semicolon+1));
> 	}

Shouldn't those two `if' statements be in the opposite order?
Otherwise the echoed commands will be a bit confusing, I think.

Also:	s/+1/ + 1/

> 	if (word_count == 0) {
> 		alias_key = "EMPTY";
> 		alias_copy_start = 0;
> 	} else if (MR_trace_is_number(words[0], &n)) {
> 		alias_key = "NUMBER";
> 		alias_copy_start = 0;
> 	} else {
> 		alias_key = words[0];
> 		alias_copy_start = 1;
> 	}
> 
> 	if (MR_trace_lookup_alias(alias_key, &alias_words, &alias_word_count))
> 	{
> 		MR_ensure_big_enough(alias_word_count, word, const char *,
> 			MR_INIT_WORD_COUNT);
> 
> 		/* Move the original words (except the alias key) up. */
> 		for (i = word_count - 1; i >= alias_copy_start; i--) {
> 			words[i + alias_word_count - alias_copy_start]
> 				= words[i];
> 		}
> 
> 		/* Move the alias body to the words array. */
> 		for (i = 0; i < alias_word_count; i++) {
> 			words[i] = alias_words[i];
> 		}
> 
> 		word_count += alias_word_count - alias_copy_start;
> 	}

Those lines could be put into a separate function called say
MR_expand_aliases().

> 	} else if (streq(words[0], "step")) {
[... 17 lines ...]
> 	} else if (streq(words[0], "goto")) {
[... 18 lines ...]
> 	} else if (streq(words[0], "finish")) {
[... 25 lines ...]
[... etc. ...]

Each of these cases should be a separate function, IMHO.

> 	} else if (streq(words[0], "printlevel")) {
> 		if (word_count == 2) {
> 			if (streq(words[1], "none")) {
> 				MR_default_print_level = MR_PRINT_LEVEL_NONE;
> 				printf("Default print level set to none.\n");
> 			} else if (streq(words[1], "some")) {
> 				MR_default_print_level = MR_PRINT_LEVEL_SOME;
> 				printf("Default print level set to some.\n");
> 			} else if (streq(words[1], "all")) {
> 				MR_default_print_level = MR_PRINT_LEVEL_ALL;
> 				printf("Default print level set to all.\n");

s/set to none./set to `none'./
s/set to some./set to `some'./
s/set to all./set to `all'./

> 			} else {
> 				MR_trace_help_cat_item("parameter", "printlevel");
> 			}
> 		} else if (word_count == 1) {
> 			printf("The default print level is ");
> 			switch (MR_default_print_level) {
> 				case MR_PRINT_LEVEL_NONE:
> 					printf("none.\n");
> 					break;
> 				case MR_PRINT_LEVEL_SOME:
> 					printf("some.\n");
> 					break;
> 				case MR_PRINT_LEVEL_ALL:
> 					printf("all.\n");

Likewise.

> 	} else if (streq(words[0], "scroll")) {
> 		if (word_count == 2) {
> 			if (streq(words[1], "off")) {
> 				MR_scroll_control = FALSE;
> 				printf("Scroll control disabled\n");

For consistency with the stuff above, "\n" should be ".\n".
Same comment applies in lots of other places.

> 	} else if (streq(words[0], "alias")) {
> 		if (word_count == 1) {
> 			MR_trace_print_all_aliases(stdout);
> 		} else if (word_count == 2) {
> 			MR_trace_print_alias(stdout, words[1]);
> 		} else {
> 			if (MR_trace_valid_command(words[2])) {
> 				MR_trace_add_alias(words[1],
> 					words+2, word_count-2);
> 				MR_trace_print_alias(stdout, words[1]);
> 			} else {
> 				printf("%s is not a valid command\n", words[2]);
> 			}
> 		}
> 	} else if (streq(words[0], "unalias")) {
> 		if (word_count == 2) {
> 			if (MR_trace_remove_alias(words[1])) {
> 				printf("Alias %s removed.\n", words[1]);
> 			} else {
> 				printf("Alias %s cannot be removed, since it does not exist.\n", words[1]);

s/%s/`%s'/g

> 	} else if (streq(words[0], "document_category")) {
...
> 			msg = MR_trace_add_cat(words[2], slot, help_text);
> 			if (msg != NULL) {
> 				printf("Document category %s not added: %s.\n",

The first "%s" here should be "`%s'".

> 	} else if (streq(words[0], "document")) {
...
> 				printf("Document item %s in category %s"

s/%s/`%s'/g


> static void
> MR_trace_list_vars(const MR_Stack_Layout_Label *top_layout, Word *saved_regs,
> 	int ancestor_level)
> {
...
> 	if (var_count < 0) {
> 		printf("mdb: there is no information about live variables\n");
> 		return;
> 	} else if (var_count == 0) {
> 		printf("mdb: there are no live variables\n");
> 		return;
> 	}

The error messages are inconsistent: here they are prefixed with "mdb: ",
but for MR_trace_retry() they are not.

It would be better for all error messages to go through a single function
called e.g. `MR_trace_report_error' or `MR_trace_err_printf';
this function could add the "mdb: " prefix, and it could potentially
print the results to somewhere other than stdout.

> static void
> MR_trace_source(const char *filename)
> {
> 	FILE	*fp;
> 
> 	if ((fp = fopen(filename, "r")) != NULL) {
> 		MR_trace_source_from_open_file(fp);
> 		fclose(fp);
> 	} else {
> 		perror(filename);
> 		/*
> 		** We actually want to write to stdout, but the following
> 		** is too Unix-specific:
> 
> 		if (errno < sys_nerr) {
> 			printf("%s: %s\n", filename, sys_errlist[errno]);
> 		} else {
> 			printf("Cannot open %s: unknown error\n", filename);
> 		}
> 		*/

Use the ANSI C function `strerror()'.

There is one non-ANSI C system which we port to, namely SunOS 4.x,
so the configure script checks for strerror(); there is a definition
of strerror() in mercury_prof.c, but you'll need to add a prototype
for it to one of our header files.

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