for review: stack dumps.

[Fergus Henderson]

On 03-Mar-1998, Tyson Dowd <trd at cs.mu.OZ.AU> wrote:
> This adds stack traces (well, at the moment, just a "stack dump") to
> the library and runtime system.
> 
> You need to compile with MCFLAGS = --stack-trace and
> MGNUCFLAGS = -DMR_STACK_TRACE (or EXTRA_MCFLAGS and EXTRA_MGNUCFLAGS)

`mgnuc' should support a `--stack-trace' option (which would
just enable -DMR_STACK_TRACE).

> to obtain an executable capable of displaying a stack dump.  This isn't
> intended to be the final mechanism -- probably a new grade or debugging
> grade would be the best solution.

Do you need to compile the whole thing (library, runtime, etc.)
with MR_STACK_TRACE defined, or does it work OK if only part
of the program is compiled with this flag?

Adding a new grade is not that hard.
If this change introduces a new incompatible binary format,
then you must at least change runtime/mercury_grade.h to
include this flag in the mangled grade identifier MR_GRADE
(which is used to ensure that the linker will catch attempts
to link incompatible objects).

> --- require.m	1998/01/23 12:33:31	1.17
> +++ require.m	1998/03/03 06:12:41
> @@ -47,7 +47,10 @@
> +	dump_stack(MR_succip, MR_sp);

Should be `MR_dump_stack'.
           ^^^

> --- mercury_goto.h	1998/01/06 07:05:59	1.5
> +++ mercury_goto.h	1998/02/19 05:21:38
> @@ -11,6 +11,7 @@
>  
>  #include "mercury_types.h"	/* for `Code *' */
>  #include "mercury_debug.h"	/* for debuggoto() */
> +#include "mercury_stack_trace.h"
>  #include "mercury_accurate_gc.h"

Why does mercury_goto.h need to include mercury_stack_trace.h and
mercury_accurate_gc.h?

> -#if defined(SPEED) && !defined(DEBUG_GOTOS) && !defined(NATIVE_GC)
> +#if defined(SPEED) && !defined(DEBUG_GOTOS) && !defined(MR_INSERT_LABELS)
>  #define	make_label(n, a, l)	/* nothing */
>  #else
>  #define	make_label(n, a, l)	make_entry(n, a, l)
>  #endif

Shouldn't defined(DEBUG_GOTOS) or !defined(SPEED) imply MR_INSERT_LABELS?
In other words, shouldn't this just be

	#if defined(MR_INSERT_LABELS)
	...

?

Likewise for most of the other #ifs in this diff (and the one for the
compiler directory) which test MR_INSERT_LABELS and also something else.

What does MR_INSERT_LABELS mean?  This should be documented somewhere
(runtime/CFLAGSFILE is one possible place; the documentation there
is rather out-of-date, though).

> /*
> ** MR_Determinsim encodes the determinism using properties of the
> ** determinsim. The bit encoding is done in stack_layout.m, changes
> ** here will need to be reflected there.
> */
> typedef Word MR_Determinsim;

Fix the misspelling of `Determinism'.

> #define MR_DETERMINSIM_IS_DET_CODE_MODEL(detism)	\
> 		(!((detism) & 1))

Ditto.

> #ifdef MR_USE_STACK_LAYOUTS
>  #define MR_MAKE_STACK_LAYOUT_ENTRY(l) 					\

Please indent stuff nested in #if by two spaces, not one
(as specified in section 5.3.1 of the Mercury developers C coding
guidelines ;-).

> void dump_stack(Code *success_pointer, Word *stack_pointer)
> {

Should be `MR_dump_stack'.

Our layout convention is that the `void' should be on a line of its own.

	void
	dump_stack(Code *success_pointer, Word *stack_pointer)
	{

Is that stack_pointer the det_stack_pointer, or the nondet_stack_pointer?

> 	int number, determinisim;

Another interesting misspelling of `determinism' ;-)

> 
> #ifndef MR_USE_STACK_LAYOUTS
> 	fprintf(stderr, "Stack dump not available in this grade.\n");
> #else
> 	fprintf(stderr, "Stack dump follows:\n");
> 
> 	do {
> 		label = lookup_label_addr(success_pointer);
> 		if (label == NULL) {
> 			fatal_error("internal label not found");
> 		}
> 
> 		layout = (Word) label->e_layout;
> 		entry_layout = MR_CONT_STACK_LAYOUT_GET_ENTRY_LAYOUT(layout);
> 		
> 		label = lookup_label_addr(
> 			MR_ENTRY_STACK_LAYOUT_GET_LABEL_ADDRESS(entry_layout));
> 		if (label == NULL) {
> 			fatal_error("entry label not found");
> 		}
> 
> 		location = MR_ENTRY_STACK_LAYOUT_GET_SUCCIP_LOC(entry_layout);
> 		type = MR_LIVE_LVAL_TYPE(location);
> 		number = MR_LIVE_LVAL_NUMBER(location);
> 
> 		determinisim = MR_ENTRY_STACK_LAYOUT_GET_DETERMINISM(
> 			entry_layout);
> 
> 		if (MR_DETERMINSIM_IS_DET_CODE_MODEL(determinisim)) {
> 			fprintf(stderr, "\t%s\n", label->e_name);
> 			if (type == MR_LVAL_TYPE_STACKVAR) {
> 				success_pointer = (Code *) field(0, 
> 					stack_pointer, -number);
> 			} else {
> 				fatal_error("can only handle stackvars");
> 			}
> 			stack_pointer = stack_pointer - 
> 				MR_ENTRY_STACK_LAYOUT_GET_NUM_SLOTS(
> 					entry_layout);
> 		}
> 	} while (MR_DETERMINSIM_IS_DET_CODE_MODEL(determinisim));

This is only dumping the det stack, I think.
The documentation in the log message etc. should make this clear.
Also the message "stack dump follows" is perhaps a bit misleading.

mercury_stack_trace.h:
> extern void dump_stack(Code *success_pointer, Word *stack_pointer);

Some documentation for this function would be helpful.

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