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

[Fergus Henderson]

On 14-Oct-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> > And that at long last completes this round of reviewing for me.
> > I'd like to see a relative diff next time.  
> 
> Most of the diff is dull, so I won't include it here; it is in the file
> DIRDIFF in ~zs/mer/ws5 on hydra.
...
from DIRDIFF:
> +BUG FIX: ! MR_STATIC_CODE_ADDRESSES and MR_USE_STACK_LAYOUTS
> +
> +runtime/mercury_conf_param.h:
> +	Move the definition of MR_STATIC_CODE_ADDRESSES before the definition
> +	of MR_USE_STACK_LAYOUTS, since MR_USE_STACK_LAYOUTS depends on whether
> +	MR_STATIC_CODE_ADDRESSES is defined. This fixes a bug introduced
> +	on september 15.

That fix is already committed, I think, so you should include
that in the log message for these other changes.

> +BUG FIX: REMOVE BROWSER/*.M
> +
> +configure.in:
> +	When removing .c files generated by the C compiler, remove those
> +	in the browser directory as well as the compiler, library and
> +	profiler directories.

s/*.M/*.c/

> diff -u //home/mercury1/zs/2oct98/library/io.m library/io.m
> --- //home/mercury1/zs/2oct98/library/io.m	Fri Oct  2 12:30:07 1998
> +++ library/io.m	Wed Oct 14 18:05:24 1998
> @@ -1029,12 +1029,20 @@
>  :- pred io__set_op_table(ops__table, io__state, io__state).
>  :- mode io__set_op_table(di, di, uo) is det.
>  
> +% For use by the debugger:
> +
> +:- pred io__get_io_input_stream_type(type_info, io__state, io__state).
> +:- mode io__get_io_input_stream_type(out, di, uo) is det.
> +
> +:- pred io__get_io_output_stream_type(type_info, io__state, io__state).
> +:- mode io__get_io_output_stream_type(out, di, uo) is det.

These predicates don't strictly need to be declared in the interface;
the `pragma export' declarations are enough.  They will after all
be declared in the automatically generated C header file.

> +++ library/std_util.m	Wed Oct 14 16:01:45 1998
...
> +	% This predicate returns the type_info for the type std_util:type_info.
> +	% It is intended for use from C code, since Mercury code can access
> +	% this type_info easily enough even without this predicate.
> +:- pred get_type_info_for_type_info(type_info).
> +:- mode get_type_info_for_type_info(out) is det.

Likewise.  

> +			/* XXX We must pass NULL here because the registers */
> +			/* have not been saved. This is probably a bug; */
> +			/* Tyson should look into it. */

Comments should be formatted as

	/*
	** ...
	*/

> +++ runtime/mercury_trace_base.c	Wed Oct 14 20:42:08 1998
> -			hfp = fopen(".mercury_histogram", "w");
> +			hfp = fopen(MR_TRACE_HISTOGRAM_FILENAME, "w");
>  			if (hfp != NULL) {
...
>  			}
>  		}

There should be an `else' case here which prints an error message
if fopen() failed.

> +++ trace/mercury_trace_help.c	Wed Oct 14 18:12:18 1998
...
> ** Before we call the C functions automatically generated by the Mercury
> ** compiler for the predicates we want to call, we call the function
> ** MR_copy_saved_regs_to_regs. This puts the contents in the saved_regs array
> ** (which includes all the control registers of the Mercury virtual machine,
> ** and which was saved on entry to the tracing system) into both the machine
> ** registers and into the MR_fake_reg array.
> **
> ** We do not save the registers anywhere after these functions return.
> ** We are not interested in the contents of the general registers and MR_succip
> ** at the return points, as we will restore the registers from saved_reg
> ** before the tracing system gives control back to the program. The call
> ** must leave the contents of MR_sp, MR_maxfr and MR_curfr unchanged.
> ** It may push new cells onto the heap or the trail, but we discard those
> ** cells (by not saving the new values of the heap and trail pointers),
> ** *after* copying any cells we want to keep to non-heap storage.

I thought about this a bit more.
I don't think this approach will work, because MR_global_hp, the heap
pointer for the "global" (permanent) heap, is one of the control
registers of the Mercury virtual machine.

>  const char *
> -MR_trace_add_cat(const char *category, int slot, const char *text)
> +MR_trace_add_cat(const char *category, int slot, const char *text,
> +	Word *saved_regs, int max_mr_num)
>  {
>  	Word	path;
>  
> *	MR_trace_help_ensure_init(saved_regs, max_mr_num);
> *	MR_copy_saved_regs_to_regs(max_mr_num, saved_regs);
>  	path = list_empty();
> *	save_transient_registers();
> *	return MR_trace_help_add_node(path, category, slot, text,
> *		saved_regs, max_mr_num);
>  }

As well as the problem with MR_copy_saved_regs_to_regs() and MR_global_hp,
the call to list_empty() is still problematic, and
the call to save_transient_registers() will still save
bogus values.  You need to add a call to restore_transient_registers()
before the call to list_empty().

>  const char *
>  MR_trace_add_item(const char *category, const char *item, int slot,
> -	const char *text)
> +	const char *text, Word *saved_regs, int max_mr_num)
>  {
>  	Word	path;
>  
> -	MR_trace_help_ensure_init();
> +	MR_trace_help_ensure_init(saved_regs, max_mr_num);
> +	MR_copy_saved_regs_to_regs(max_mr_num, saved_regs);
>  	path = list_empty();
>  	path = list_cons(category, path);
> -	return MR_trace_help_add_node(path, item, slot, text);
> +	save_transient_registers();
> +	return MR_trace_help_add_node(path, item, slot, text,
> +		saved_regs, max_mr_num);
>  }

Likewise.

> +MR_trace_help_add_node(Word path, const char *name, int slot, const char *text,
> +	Word *saved_regs, int max_mr_num)
>  {
>  	Word	result;
>  	char	*msg;
>  
> +	MR_copy_saved_regs_to_regs(max_mr_num, saved_regs);
>  	ML_HELP_add_help_node(MR_trace_help_system, path, slot,
>  		(String) (Word) name, (String) (Word) text,
>  		&result, &MR_trace_help_system);
> +
> +	MR_copy_saved_regs_to_regs(max_mr_num, saved_regs);
> +	MR_trace_help_system = MR_make_permanent(MR_trace_help_system,
> +				MR_trace_help_system_type);
> +
> +	MR_copy_saved_regs_to_regs(max_mr_num, saved_regs);

You should delete the second call to MR_copy_saved_regs_to_regs(),
since that call will reset the heap pointer and thus effectively
deallocate the value pointed to be MR_trace_help_system before
it has been made permanent.

Same applies to the third call to MR_copy_saved_regs_to_regs(),
since that will deallocate the value pointed to by `result',
which is used later.

>  void
> -MR_trace_help_cat_item(const char *category, const char *item)
> +MR_trace_help_cat_item(const char *category, const char *item,
> +	Word *saved_regs, int max_mr_num)
>  {
>  	Word	path;
>  	Word	result;
>  	char	*msg;
>  
> -	MR_trace_help_ensure_init();
> +	MR_trace_help_ensure_init(saved_regs, max_mr_num);
>  	path = list_empty();
>  	path = list_cons(item, path);
>  	path = list_cons(category, path);
> +	save_transient_registers();
>  	ML_HELP_path(MR_trace_help_system, path, MR_trace_help_stdout, &result);
> +	MR_copy_saved_regs_to_regs(max_mr_num, saved_regs);
>  	if (ML_HELP_result_is_error(result, &msg)) {
>  		printf("internal error in the trace help system: %s\n", msg);
>  	}
>  }

Again you need a call to restore_transient_registers() before the
call to list_empty().

The call to MR_copy_saved_regs_to_regs() effectively deallocates
the memory pointed to by `result' before it is used.

I don't understand why the call to MR_copy_saved_regs_to_regs()
is where it is.

>  static void
> +MR_trace_help_ensure_init(Word *saved_regs, int max_mr_num)
>  {
> +	static	bool	done = FALSE;
> +	Word		typeinfo_type;
> +	Word		output_stream_type;
> +
> +	if (! done) {
> +		MR_copy_saved_regs_to_regs(max_mr_num, saved_regs);
> +		ML_get_type_info_for_type_info(&typeinfo_type);
> +		ML_HELP_help_system_type(&MR_trace_help_system_type);
> +		MR_trace_help_system = MR_make_permanent(
> +					MR_trace_help_system_type,
> +					typeinfo_type);
>  
> +		MR_copy_saved_regs_to_regs(max_mr_num, saved_regs);
>  		ML_HELP_init(&MR_trace_help_system);
> +
> +		MR_copy_saved_regs_to_regs(max_mr_num, saved_regs);
> +		MR_trace_help_system = MR_make_permanent(MR_trace_help_system,
> +					MR_trace_help_system_type);
> +
> +		MR_copy_saved_regs_to_regs(max_mr_num, saved_regs);
> +		ML_io_output_stream_type(&output_stream_type);
>  		ML_io_stdout_stream(&MR_trace_help_stdout);
> +		MR_trace_help_stdout = MR_make_permanent(MR_trace_help_stdout,
> +					output_stream_type);

Again there are problems here with the calls to MR_copy_saved_regs_to_regs();
they may deallocate memory before it is used.

> +++ trace/mercury_trace_internal.c	Wed Oct 14 20:08:35 1998
...
>  static void
>  MR_trace_internal_ensure_init(void)
>  {
...
> -		printf(MR_trace_banner);
> +		if (getenv("MERCURY_SUPPRESS_MDB_VERSION") != NULL) {
> +			printf(MR_trace_banner, "VERSION");
> +		} else {
> +			printf(MR_trace_banner, MR_VERSION);
> +		}
> +

I don't understand what that code is trying to do.
What's the point of the undocumented MERCURY_SUPRESS_MDB_VERSION
environment variable?

>  		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");
> +		MR_trace_internal_init_from_env(
> +			"DEFAULT_MERCURY_DEBUGGER_INIT");

Hmm, shouldn't you print an error message if all four steps fail?

> +	/* XXX This code is too Unix specific. */
>  	buf = checked_malloc(strlen(env) + strlen(filename) + 2);
>  	(void) strcpy(buf, env);
>  	(void) strcat(buf, "/");

I would really much prefer you to 

> +#ifdef	MR_TRACE_HISTOGRAM
>  	} else if (streq(words[0], "histogram_all")) {
>  		if (word_count == 2) {
>  			FILE	*fp;
>  
>  			fp = fopen(words[1], "w");
>  			if (fp == NULL) {
> -				perror(words[1]);
> +				fprintf(stderr, "mdb: cannot open file `%s' "
> +					"for output: %s.",
> +					words[1], strerror(errno));

Missing `\n'.

>  			} else {
>  				MR_trace_print_histogram(fp, "All-inclusive",
>  					MR_trace_histogram_all,
>  					MR_trace_histogram_hwm);
> -				fclose(fp);
> +				if (fclose(fp) != 0) {
> +					fprintf(stderr, "mdb: error closing "
> +						"file `%s': %s.",
> +						words[1], strerror(errno));
> +				}

Ditto.

> +			fp = fopen(words[1], "w");
>  			if (fp == NULL) {
> -				perror(words[1]);
> +				fprintf(stderr, "mdb: cannot open file `%s' "
> +					"for output: %s.",
> +					words[1], strerror(errno));

Ditto.

>  			} else {
>  				MR_trace_print_histogram(fp, "Experimental",
>  					MR_trace_histogram_exp,
>  					MR_trace_histogram_hwm);
> -				fclose(fp);
> +				if (fclose(fp) != 0) {
> +					fprintf(stderr, "mdb: error closing "
> +						"file `%s': %s.",
> +						words[1], strerror(errno));
> +				}

Ditto.

>  	} else if (streq(words[0], "source")) {
>  		if (word_count == 2) {
> -			MR_trace_source(words[1]);
> +			/* If it fails, the error message */
> +			/* will have already been printed. */
> +			(void) MR_trace_source(words[1]);

Non-standard comment formatting.

> +++ doc/user_guide.texi	Wed Oct 14 19:55:40 1998
> @@ -2782,13 +2782,17 @@
>  * Debugger commands::
>  @end menu
>  
> + at c XXX
> +This chapter of the user guide is still under construction.

s/user guide/user's guide/

> @@ -2957,6 +2964,8 @@
>  mdb @var{prog} @var{arg1} ...
>  @end example
>  
> +XXX more to come

That should be "@c XXX ...".

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

The stuff involving MR_copy_saved_regs_to_regs() will need another
review, but apart from that, it looks OK.

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