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

[Zoltan Somogyi]

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

No, the diff (trace_stack_layout = no) only turns off the parts of the stack
layout that are needed only for execution tracing. The parts needed for stack
tracing are still there; the change does not turn off basic_stack_layout
or procid_stack_layout.

The comment was ambiguous, saying only "tracing"; I have modified it
to read

	% Some predicates in the builtin modules are missing
	% typeinfo arguments, which means that execution
	% tracing will not work on them. Predicates defined
	% there should never be part of an execution trace
	% anyway; they are effectively language primitives.
	% (They may still be parts of stack traces.)

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

As I said above, this change does not prevent that.

Actually, a bigger problem is solutions, which at the moment does not
register a stack layout unless compiling in the debug grade. This cuts short
some stack traces. I intend to fix this (for solutions and maybe for other
similar primitives) later.

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

Ditto.

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

The log message wasn't worded very well. The addition of the stack dump
command in the debugger means that being able to do a stack trace is now
required for execution tracing to be fully functional. Now it reads:

The next batch of changes have to do with adding a stack dump command
to the debugger. Since debugging is possible even in non-debug grades,
this in turn requires allowing stack tracing to work in non-debug grades,
on programs in which only some modules are compiled with execution
(and hence stack) tracing.

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

Execution tracing needs stack layouts because of the new "d" command.
Stack layouts have always needed MR_INSERT_LABELS; it causes the stack
layout addresses to be inserted into the label table.

> > -:- 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__st
       ate).
> > +:- 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.

OK.

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

Yes, that is a good idea, but that is not part of this change.

> > +		( { 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.

OK.

> >  /*
> > +** 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.

This now reads "...before any Mercury code is run."

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

Even over consistency with other code nearby?

> > +	if (raw_word_count > 0 && isdigit(*raw_words[1])) {
> 
> For portability, the call to isdigit() needs to cast the
> argument to `unsigned char':

I have instead defined macros MR_isdigit and MR_isspace that do the cast,
and used those consistently; this avoids lots of too long lines.

Zoltan.