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

[Fergus Henderson]

On 01-Oct-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> >>> ENSURE THAT INPUT ARGUMENTS ARE ALWAYS VISIBLE <<<

That change looks fine.

> >>> START NUMBERING FRAMEVARS FROM ONE <<<

That change looks fine.

> >>> IMPLEMENT NEW DEBUGGER COMMAND SET <<<
> 
> runtime/mercury_stack_trace.[ch]:
> 	Factor out the code that prints the id of a procedure into a function
> 	of its own, so that it can also be used from the debugger, ensuring
> 	appearance commonality.
> 
> 	Add more documentation in the header file.
> 
> trace/mercury_trace_internal.c:
> 	Implement the proposed command set. Command names are now words,
> 	and several commands now have options allowing the user to override
> 	the default print level or strictness of the command, or the
> 	invocation conditions or action of a break point. Allows control
> 	over command echoing and the scrolling of sequences of event reports.
> 	Supports aliases, command file sourcing etc. Implements the retry
> 	command, using the info in the fixed stack slots.
> 
> trace/mercury_trace.[ch]:
> 	Extend the trace controls to support the new functionalities
> 	required by the new debugger language, which are print levels,
> 	variable-strictness commands, a more flexible finish command,
> 	and the retry command.
> 
> 	Pass the command structure to MR_trace_event_report, since
> 	the user can now forcibly terminate the scrolling of reports.
> 
> trace/mercury_trace_alias.[ch]:
> 	New module to manage aliases for the debugger.
> 
> trace/mercury_trace_spy.[ch]:
> 	New module to manage break points. The test of whether an event
> 	matches a break point is now much more efficient than before.
> 	The new module also allows several breakpoints with different
> 	actions and different invocation conditions (e.g. all ports,
> 	entry port, interface ports or specific (possibly internal) port)
> 	to be defined on the same procedure.
> 
> trace/mercury_trace_tables.[ch]:
> 	New module to manage a table of the debuggable modules, in which
> 	each such module is linked to the list of the layouts of all the
> 	procedures defined in that module. This information allows the
> 	debugger to turn the name of a predicate/function (possibly together
> 	with its arity and mode number) into the procedure layout structure
> 	required by the spy point module. Eventually it may also be useful
> 	in supplying lists of identifiers for command line completion.
> 
> 	Modules for which no stack layout information is available will
> 	not be included in the table, since do_init_modules will not
> 	register any labels for them in the label table.
> 
> trace/mercury_macros.h:
> 	A new file holding macros that can be useful in more than one module.

Hmm, there are lots of macros defined in header files other than this one.
Grouping procedures based on their nature (e.g. "macro") will not give
you strong cohesion.  It is better to group procedures that have
a related purpose.

I notice that all the macros in this file deal with arrays
(resizing them, searching them, inserting into them, etc.).
How about renaming this as `mercury_array_macros.h'?

Also, I think this header should probably go in `runtime' rather than
`trace', since we will probably want to use these macros from code in
the runtime sooner or later...

> Index: trace/mercury_trace_alias.c
...
> +/*
> +** mercury_trace_alias.h

It's `.c' not `.h'.
Also you should have a comment explaining the purpose of this file.

mercury_trace_help.c:
> +#include "mercury_imp.h"
> +#include "mercury_trace_help.h"
> +#include "mercury_macros.h"
> +#include "mercury_misc.h"
> +#include "mercury_deep_copy.h"
> +#include "help.h"
> +#include "io.h"
> +#include <stdio.h>

Those header file names "help.h" and "io.h" are an invasion
of the user's header file name space.

For the moment I suggest you just add an "XXX" comment.

> +const char *
> +MR_trace_add_item(const char *category, const char *item, int slot,
> +	const char *text)
> +{
> +	Word	path;
> +
> +	MR_trace_help_ensure_init();
> +	path = list_empty();
> +	path = list_cons(category, path);
> +	return MR_trace_help_add_node(path, item, slot, text);
> +}

The call to list_cons() won't work in non-gc grades on SPARCs,
because the `hp' register won't be valid.

In principle, the same applies to list_empty() too, for the
NUM_TAG_BITS == 0 case.

> +static void
> +MR_trace_help_make_permanent(void)
> +{
> +	save_transient_registers();
> +	MR_trace_help_system = MR_make_permanent(MR_trace_help_system,
> +				MR_trace_help_system_type);
> +	restore_transient_registers();
> +}

The call to save_transient_registers() here is wrong --
the transient registers aren't valid at the point of the call
(they get clobbered by entry to or exit from a C function),
so if you try to save them there, you will be saving garbage.

> Index: trace/mercury_trace_internal.h
...
> +#undef	USE_GCC_GLOBAL_REGISTERS

I don't think that is a good idea.  Doing this is at best fragile. 
Doing this in a header file is extrememly dangerous.
See the following extracts from the GCC manual:

 |    It is not safe for one function that uses a global register variable
 | to call another such function `foo' by way of a third function `lose'
 | that was compiled without knowledge of this variable (i.e. in a
 | different source file in which the variable wasn't declared).  This is
 | because `lose' might save the register and put some other value there.
 | For example, you can't expect a global register variable to be
 | available in the comparison-function that you pass to `qsort', since
 | `qsort' might have put something else in that register.  (If you are
 | prepared to recompile `qsort' with the same global register variable,
 | you can solve this problem.)
...
 |    A function which can alter the value of a global register variable
 | cannot safely be called from a function compiled without this variable,
 | because it could clobber the value the caller expects to find there on
 | return.  Therefore, the function which is the entry point into the part
 | of the program that uses the global register variable must explicitly
 | save and restore the value which belongs to its caller.

At very least, you should have a comment explaining the reason for this,
plus some very LOUD comments pointing out the restrictions on
what code that #includes this file is allowed to do.

But I think it would be much much better to just delete that line.

> +++ mercury_trace_spy.h	Tue Sep 22 12:30:04 1998
...
> +#define	MR_spy_action_string(a)		((a == MR_SPY_STOP) ? "stop" :      \
> +					(a == MR_SPY_PRINT) ? "print" :     \
> +					"weird spy action")

Tyson already picked this one up, but I suggest that you do a global
search for "weird" and replace it with "unknown".

> +typedef struct struct_mr_spy_point MR_Spy_Point;
> +
> +struct struct_mr_spy_point {

s/struct_mr_spy_point/MR_spy_point_struct/

> +extern	bool		MR_event_matches_spy_point(const MR_Stack_Layout_Label
> +				*layout, MR_Trace_Port port,
> +				MR_Spy_Action *action);
> +
> +extern	MR_Spy_Point	*MR_add_spy_point(MR_Spy_When when,
> +				MR_Spy_Action action,
> +				const MR_Stack_Layout_Entry *entry,
> +				const MR_Stack_Layout_Label *label,
> +				const char *path);

You should document these functions.

> +++ mercury_trace_tables.c	Mon Sep 21 20:03:43 1998
...
> +#undef	USE_GCC_GLOBAL_REGISTERS

See above.

> +void
> +MR_dump_module_procs(FILE *fp, const char *name)
> +{
> +	MR_Module_Info		*module;
> +	const MR_Proc_Node	*cur;
> +
> +	module = MR_search_module_info(name);
> +	if (module == NULL) {
> +		fprintf(fp, "There is no debugging info about module %s\n",

s/%s/`%s'/

> +				name);
> +	} else {
> +		fprintf(fp, "List of procedures in module %s\n\n", name);

s/%s/`%s'/

> +++ mercury_trace_tables.h	Tue Sep 22 12:30:04 1998
> +typedef struct struct_mr_proc_node	MR_Proc_Node;
> +
> +struct struct_mr_proc_node {

s/struct_mr_proc_node/MR_proc_node_struct/g

Otherwise, that change ("IMPLEMENT NEW DEBUGGER COMMAND SET") 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.