[m-dev.] for review: stack dumps in the debugger and a bug fix

[Fergus Henderson]

On 07-Jun-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> compiler/liveness.m:
> 	Turn off type_info liveness for builtins without typeinfos.
> 	Since these builtins establish no gc points and shouldn't be
> 	execution traced, this is OK.

OK so far...

> compiler/mercury_compile.m:
> 	Turn off execution tracing for the modules builtin.m and
> 	private_builtin.m.

I think the diff turns off stack tracing too, not just execution tracing.

>       The latter contains the interface predicates
> 	for the builtins without typeinfos. Since the interface predicates
> 	also lack the typeinfos, the compiler would get an internal abort
> 	if we left execution tracing on.
> 
> 	In any case, these two modules contain stuff that users should
> 	consider language builtins, which means they should not be traced.

What about predicates such as

	table_loopcheck_error,
	builtin_compare_pred,
	builtin_compare_non_canonical,

which call error/1 -- surely the user will want a stack dump in those
cases.

Also if the user has a type with a user-defined equality, then unify/2
could call user code, and again the user may want a stack trace in that
case.

> The next batch of changes have to do with allowing execution tracing
> to work in the usual (non-debug) grades, on programs in which only
> some modules are compiled with tracing.

Did you mean stack tracing rather than execution tracing?

Execution tracing seems to work in the usual grades already.

> compiler/llds_out.m:
> compiler/mercury_compile.m:
> runtime/mercury_conf_param.h:
...
> 	The new version of llds_out takes a parameter that says whether
> 	this module is compiled with execution tracing or not, and if it is,
> 	it generates a #define MR_LABELS_FOR_EXEC_TRACE *before* the #include
> 	of mercury_imp.h. This causes mercury_conf_param.h, included from
> 	mercury_imp.h, to define the macros MR_USE_STACK_LAYOUTS and
> 	and MR_INSERT_LABELS, which in turn cause stack layouts for this
> 	module to be handled as if the grade was debug.

Why does execution tracing need stack layouts and MR_INSERT_LABELS?

> Index: compiler/llds_out.m
...
>  	% Given a 'c_file' structure, open the appropriate .c file
> -	% and output the code into that file.
> +	% and output the code into that file. The bool says whether
> +	% this Mercury module was compiled with any flavor of execution
> +	% tracing; the third argument gives the set of labels that have
> +	% layout structures.
>  
> -:- pred output_c_file(c_file, set_bbbtree(label), io__state, io__state).
> -:- mode output_c_file(in, in, di, uo) is det.
> +:- pred output_c_file(c_file, bool, set_bbbtree(label), io__state, io__state).
> +:- mode output_c_file(in, in, in, di, uo) is det.

I think it would make more sense for output_c_file to get this bool
directly from the globals structure in the io__state rather than
passing it as a parameter.  After all, in general there may 
be lots of other options that it would like to check.

For example, it would be a good idea for it to emit macros
in the generated code for all the options that affect binary
compatibility of the generated C code, and for mercury_conf_param.h
to use these to check that the options you're using to compile
the C file are consistent with those used to generate the C file.

> @@ -194,7 +198,13 @@
>  		io__write_string("\n"),
>  		io__write_string("ENDINIT\n"),
>  		io__write_string("*/\n\n"),
> -		io__write_string("#include ""mercury_imp.h""\n"),
> +		( { ExecTrace = yes } ->
> +			io__write_string("#define MR_LABELS_FOR_EXEC_TRACE\n"),
> +			io__write_string("#include ""mercury_imp.h""\n"),
> +			io__write_string("#include ""mercury_trace.h""\n")
> +		;
> +			io__write_string("#include ""mercury_imp.h""\n")
> +		),
...
> +		),
> +		( { ExecTrace = yes } ->
> +			io__write_string("#define MR_LABELS_FOR_EXEC_TRACE\n"),
> +			io__write_string("#include ""mercury_imp.h""\n"),
> +			io__write_string("#include ""mercury_trace.h""\n")
> +		;
> +			io__write_string("#include ""mercury_imp.h""\n")

It would be a good idea to split this duplicate code out into a
seperate procedure.

>  /*
> +** MR_NEED_INITIALIZATION_AT_START -- the module specific initialization code
> +**				      must be run any Mercury code is run.

I think you omitted a word or two here.

> --- mercury_goto.h	1998/05/16 07:28:15	1.9
> +++ mercury_goto.h	1998/06/03 11:05:59
> @@ -18,12 +18,12 @@
>  #define entry(label) paste(_entry_,label)
>  #define skip(label) paste(skip_,label)
>  
> -#ifdef MR_USE_STACK_LAYOUTS
> +#if defined(MR_USE_STACK_LAYOUTS)

I prefer the more concise form.

> Index: runtime/mercury_stack_trace.h
...
> +/*
> +** MR_dump_stack_from_layout:
> +**	This function does the same job and makes the same assumptions
> +**	as MR_dump_stack, but instead of the succip, it takes the entry
> +**	layout of the current procedure as input. It also takes a paramater
> +**	that tells it where to put the stack dump. If the entire stack
> +**	was printed successfully, the return value is NULL; otherwise,
> +**	it is a string indicating why the dump was cut short.
> +*/

s/paramater/parameter/

> +static MR_next
> +MR_trace_debug_cmd(MR_trace_cmd_info *cmd, const MR_Stack_Layout_Label *layout,
> +	MR_trace_port port, int seqno, int depth, const char *path)
> +{
> +	char	line[MR_MAX_LINE_LEN];
> +	char	count_buf[MR_MAX_LINE_LEN];
> +	char	*raw_words[MR_MAX_LINE_LEN/2+1];

I suggest whitespace around operators (/ and +).

> +	raw_word_count = MR_trace_break_into_words(line, raw_words+1);

Ditto.

> +	if (raw_word_count > 0 && isdigit(*raw_words[1])) {

For portability, the call to isdigit() needs to cast the
argument to `unsigned char':

	if (raw_word_count > 0 && isdigit((unsigned char) *raw_words[1])) {

Otherwise, it can result in undefined behaviour on machines with
signed characters if the user types in a character with the top bit set.

> +		while (isdigit(*s)) {

Ditto.

> +static bool
> +MR_trace_is_number(char *word, int *value)
>  {
> +	if (isdigit(*word)) {
> +		*value = *word - '0';
> +		word++;
> +		while (isdigit(*word)) {
> +			*value = (*value * 10) + *word - '0';
> +			word++;
> +		}

Ditto.

> +MR_trace_break_into_words(char line[], char *words[])
...
> +		while (line[char_pos] != '\0' && isspace(line[char_pos])) {
...
> +		while (line[char_pos] != '\0' && !isspace(line[char_pos])) {

Ditto.

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