[m-dev.] For review: Stacks dump in the external debugger (round 2)

[Fergus Henderson]

On 16-Feb-1999, Erwan Jahier <Erwan.Jahier at irisa.fr> wrote:
> 
> --- mercury_trace.c	1999/02/12 00:16:42	1.5
> +++ mercury_trace.c	1999/02/16 16:26:48
> @@ -432,3 +432,28 @@
>  	*succeeded = FALSE;
>  	return 0;
>  }
> +
> +
> +/*
> +** The different Mercury determinisms are internaly represented by integers. 

s/internaly/internally/

> +** This array gives the correspondance with the internal representation and 
> +** the names that are usually used to denote determinisms.
> +*/
> +
> +extern const char * MR_detism_names[] = {
> +	"failure",	/* 0 */
> +	"",		/* 1 */
> +	"semidet",	/* 2 */
> +	"nondet",	/* 3 */
> +	"erroneous",	/* 4 */
> +	"",		/* 5 */
> +	"det",		/* 6 */
> +	"multi",	/* 7 */
> +	"",		/* 8 */
> +	"",		/* 9 */
> +	"cc_nondet",	/* 10 */
> +	"",		/* 11 */
> +	"",		/* 12 */
> +	"",		/* 13 */
> +	"cc_multi"	/* 14 */
> +};
> Index: trace/mercury_trace.h
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/trace/mercury_trace.h,v
> retrieving revision 1.5
> diff -u -r1.5 mercury_trace.h
> --- mercury_trace.h	1999/02/12 00:16:43	1.5
> +++ mercury_trace.h	1999/02/16 16:26:48
> @@ -98,6 +98,8 @@
>  	bool			MR_trace_must_check;
>  } MR_Trace_Cmd_Info;
>  
> +extern const char * MR_detism_names[];

The comment above should go on the declaration here in the header file,
instead of, or as well as, on the definition in mercury_trace.c.

> +			case MR_REQUEST_STACK_REGS:
> +				if (MR_debug_socket) {
> +					fprintf(stderr, "\nMercury runtime: "
> +						"REQUEST_STACK_REGS\n");
> +				}
> +				MR_send_message_to_socket_format(
> +					"stack_regs(\"sp = %p, curfr = %p, "
> +					"maxfr = %p\").\n",
> +					MR_saved_sp(saved_regs),
> +					MR_saved_curfr(saved_regs),
> +					MR_saved_maxfr(saved_regs));
> +				break;

The "%p" format is not guaranteed to give you a syntactically correct
Prolog term.  On some systems, "%p" will give you a hexadecimal number,
without any "0x" prefix, e.g. "0000ffff", which is not a syntactically
correct Prolog term.

I suggest you use "%lu", and cast to `unsigned long'.

However, even better would be to put something like

	typedef unsigned long MR_intmax_t;
	#define MR_PRINTF_INTMAX_T "%lu"

in somewhere like runtime/mercury_types.h and then use those.

> -static void
> -MR_send_message_to_socket_format(const char *format, const char *message)
> +void    
> +MR_send_message_to_socket_format(const char *format, ...)

Any particular reason why this became non-static?

Sorry if my comments about "in header file", "in C file" confused you,
but if this function isn't used from any other module, then there
is no need to make it extern and declare it in the header file,
instead it should remain static and the declaration should be moved
from the header file to the start of the C file.

> +static void
> +MR_dump_stack_record_print_to_socket(FILE *fp, 
> +	const MR_Stack_Layout_Entry *entry_layout, int count, int start_level, 
> +	Word *base_sp, Word *base_curfr)
> +{
> +	MR_send_message_to_socket_format( "%d.\n ", start_level);

Why the space after the \n ?

> +	if (count > 1) {
> +		MR_send_message_to_socket_format( " %d*.\n ", count);

"1*." is not valid syntax for a Prolog term.

> +	} else if ((base_sp == NULL) && (base_curfr == NULL)) {
> +		MR_send_message_to_socket_format( "%s.\n ", "r1");
> +	}

Why does it send "r1." in this case?  A comment might help.
Also why use ("%s.\n", "r1") instead of just ("r1.\n")?

> +	if (MR_ENTRY_LAYOUT_COMPILER_GENERATED(entry)) {
> +		MR_send_message_to_socket_format(
> +			"proc(%s for %s:%s/%ld-%ld).\n",

That won't be valid syntax for a Prolog term.
I suggest

			"proc('%s for %s:%s'/%ld-%ld).\n"

which should work OK except when one of the names contains a
single quote ("'").

> +		if (strcmp(entry->MR_sle_comp.MR_comp_type_module,
> +				entry->MR_sle_comp.MR_comp_def_module) != 0)
> +		{
> +			MR_send_message_to_socket_format( " {%s}.\n",
> +				entry->MR_sle_comp.MR_comp_def_module);
> +		}

Likewise appending {...} won't be valid syntax for a Prolog term.

> +	} else {
> +		if (entry->MR_sle_user.MR_user_pred_or_func == MR_PREDICATE) {
> +			MR_send_message_to_socket("pred");
> +		} else if (entry->MR_sle_user.MR_user_pred_or_func ==
> +				MR_FUNCTION)
> +		{
> +			MR_send_message_to_socket( "func.\n");

Why do you print ".\n" for funcs but not for preds?
That is probably a mistake.

> +		MR_send_message_to_socket_format(
> +			"proc(%s:%s/%ld-%ld).\n",

I suggest

			"proc('%s:%s'/%ld-%ld).\n",
			      ^     ^

> +		if (strcmp(entry->MR_sle_user.MR_user_decl_module,
> +				entry->MR_sle_user.MR_user_def_module) != 0)
> +		{
> +			MR_send_message_to_socket_format( "{%s}.\n",
> +				entry->MR_sle_user.MR_user_def_module);
> +		}

As above, that needs fixing.

> +	MR_send_message_to_socket_format( "%s.\n", 
> +		MR_detism_names[entry->MR_sle_detism]);
> +
> +	if (extra != NULL) {
> +		MR_send_message_to_socket_format( " %s.\n", extra);
> +	}

Shouldn't that be sent as a string or an atom or something?
i.e. in quotes?

> +	MR_send_message_to_socket("end_proc");
> +}

It would be a good idea to document the protocol that is
used by mercury_trace_external.c to communicate with the external
debugger, especially since that protocol is becoming quite
complex now.

> Index: trace/mercury_trace_internal.h
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/trace/mercury_trace_internal.h,v
> retrieving revision 1.5
> diff -u -r1.5 mercury_trace_internal.h
> --- mercury_trace_internal.h	1999/02/10 22:31:22	1.5
> +++ mercury_trace_internal.h	1999/02/16 16:26:52
> @@ -48,6 +48,7 @@
>  			MR_Trace_Port port, int seqno, int depth,
>  			const char *path, int *max_mr_num);
>  
> +
>  /*
>  ** Debugger I/O streams.
>  ** Replacements for stdin/stdout/stderr respectively.

Please undo that accidental change.

I'd like to see another diff please.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "Binaries may die
WWW: <http://www.cs.mu.oz.au/~fjh>  |   but source code lives forever"
PGP: finger fjh at 128.250.37.3        |     -- leaked Microsoft memo.