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

[Fergus Henderson]

> *** USE FIXED STACK SLOTS FOR TRACE INFO ***
> 
> compiler/code_gen.m:
> compiler/code_info.m:
> compiler/live_vars.m:
> compiler/trace.m:
> 	If execution tracing is enabled, reserve the first few stack slots
> 	to hold the event number of the call event, the call number, the
> 	call depth, the redo layout structure address (if generating redo
> 	events) and the from_full flag at the time of call (if we are doing
> 	shallow tracing). By allocating the first four of these to fixed stack
> 	slots, the debugger knows where to look for them without having
> 	to be told. It finds out the location of the fifth, if needed,
> 	from a new slot in the proc layout structure. (It is not possible
> 	to allocate all five to fixed stack slots without wasting stack space
> 	in some cases.)
> 
> compiler/trace.m:
> 	Remove from the call to MR_trace the parameters that are now in fixed
> 	stack slots, since MR_trace can now look them up itself.
> 
> compiler/continuation_info.m:
> compiler/stack_layout.m:
> 	Add an extra field to the proc_layout_info. If the module is shallow
> 	traced, this field says which stack slot holds the saved value of
> 	MR_from_full. If it is not shallow traced, this field says that
> 	there is no such stack slot.
> 
> runtime/mercury_stack_layout.h:
> 	Add macros for accessing the fixed stack slots holding the event
> 	number of the call event, the call number, the call depth, and,
> 	at a redo event, the redo layout structure address.
> 
> 	Support the new field in proc layouts that gives the location of the
> 	from-full flag (if any).
> 
> runtime/mercury_trace_base.[ch]:
> trace/mercury_trace.[ch]:
> 	Remove the call number and call depth arguments from MR_trace
> 	and its avatars, since this info is now in fixed stack slots
> 	in every procedure that can call MR_trace. This should reduce
> 	the size of the executable significantly, since there are lots
> 	of calls to MR_trace.
> 
> runtime/mercury_init.h:
> runtime/mercury_wrapper.h:
> 	Update the prototype of MR_trace_{fake,real}, and the type of the
> 	global that points to them.

continuation_info.m:
> @@ -575,6 +571,11 @@
> >>> USE FIXED STACK SLOTS FOR TRACE INFO <<<
>  	%
>  :- type proc_layout_info
...
> +			maybe(int),	% If the trace level is shallow,
> +					% this contains the number of the
> +					% stack slot containing the
> +					% value of MR_trace_full_full
> +					% at the time of the call.

s/full_full/from_full/

live_vars.m:
> +allocate_stack_slots(ColourList, CodeModel, ReservedSlots, StackSlots) :-

`NumReservedSlots' might be a better variable name.
 ^^^

stack_layout.m:
>  :- pred stack_layout__construct_proc_layout(label::in,
>  	determinism::in, int::in, maybe(int)::in, maybe(label)::in,
> -	stack_layout_info::in, stack_layout_info::out) is det.
> +	maybe(int)::in, stack_layout_info::in, stack_layout_info::out) is det.

Here -- and in lots of other places -- I think it would be clearer
if you used a typedef called say `maybe_from_full_slot' rather than
`maybe(int)', particularly if there was a comment next to the
definition of the type explaining its purpose.

trace.m:
...
> +	% The predicate returns the number of this slot if it is used,
> +	% an abstract struct that represents the tracing-specific part
> +	% of the code generator state.
> +:- pred trace__setup(globals::in, maybe(int)::out, trace_info::out,
>  	code_info::in, code_info::out) is det.

Insert "and" before "an abstract struct".

> +trace__generate_slot_fill_code(TraceInfo, TraceCode) -->
> +	( MaybeRedoLayoutSlot = yes(RedoLayoutLabel) ->
> +		trace__redo_layout_slot(CodeModel, RedoLayoutLval),
> +		trace__stackref_to_string(RedoLayoutLval, RedoLayoutStr),
> +		llds_out__make_stack_layout_name(RedoLayoutLabel,
> +			LayoutAddrStr),
> +		string__append_list([
> +			FillThreeSlots, "\n",
> +			"\t\t", RedoLayoutStr, " = (Word) (const Word *) &",
> +			LayoutAddrStr, ";"

Why the double cast?  Why not cast straight to `Word'?

> +	GotoStmt = "\t\tif (MR_jumpaddr != NULL) GOTO(MR_jumpaddr);",
> +	string__append_list([DeclStmt, SaveStmt, CallStmt, RestoreStmt,
> +		GotoStmt], TraceStmt),

Hmm, is the test of `MR_jumpaddr' needed if tracing of REDO events
is not enabled?

If not, then I think it would be worthwhile to at least add a comment
explaining this.

> +++ mercury_layout_util.c	Tue Sep 22 16:51:58 1998
> @@ -13,31 +13,24 @@
> >>> USE FIXED STACK SLOTS FOR TRACE INFO <<<
>  ** if MR_ENTRY_LAYOUT_HAS_PROC_ID(entry) evaluates to true.
>  ** Group (3) is present and meaningful
>  ** if MR_ENTRY_LAYOUT_HAS_EXEC_TRACE(entry) evaluates to true.
> +**
> +** Group (2) fields have a different interpretation if the procedure is
> +** compiler-generated. You can test for this via the macro
> +** MR_ENTRY_LAYOUT_COMPILER_GENERATED.

What's the different interpretation?

> +#define	MR_ENTRY_LAYOUT_COMPILER_GENERATED(entry)		\
> +		((int) entry->MR_sle_pred_or_func > MR_FUNCTION)

Hmm, that code is a bit obfuscated -- a comment here would be helpful.
If special values are being used as sentinels, it would probably
be a good idea to make those special values part of the enum.

> +void
> +MR_print_proc_id(FILE *fp, const MR_Stack_Layout_Entry *entry_layout,
> +	const char *extra)
> +{
...
> +	fprintf(fp, "%s %s:%s/%ld-%ld (%s)",
> +		entry_layout->MR_sle_pred_or_func == MR_PREDICATE ?
> +			"pred" : "func",

Will this incorrectly print "func" for compiler-generated procedures?
Or is it supposed to be guaranteed that this C function will never be
called for with an entry_layout whose pred_or_func field is neither
MR_PREDICATE nor MR_FUNCTION?  If the latter, then that precondition
should be documented in the header file.

Otherwise, that change ("USE FIXED STACK SLOTS FOR TRACE INFO") looks fine.

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