[m-rev.] for review: debugging tabling procedures

[Fergus Henderson]

On 08-Nov-2002, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> I want to include this change in the release. If this change is not
> in the release, the people I am working with on minimal model tabling
> (currently Kostis Sagonas and Peter Stuckey) will not be able to use
> an installed release to compile a workspace on the minimal model tabling
> branch in debugging grades.

Installing a ROTD release isn't difficult; we even provide binary
distributions of ROTD releases.  So that doesn't seem like a compelling
reason.

> The installation of this change will require workspaces compiled in debugging
> grades to be recompiled. Workspaces compiled in non-debugging grades will not
> be affected.

That one is more persuasive; if we're going to break ABI compatibility
for debug grades, it's better to do it before the release than after the
release.

However, it is still a very large amount of new code to add at this point
in the release cycle.  So I'm still somewhat dubious about the merits of
including this change in the forthcoming release.

Please don't check this one in until I've at least had a chance to review
it thoroughly.

> compiler/llds_out.m:
> 	Abstract out existing code into a new procedure to make it available
> 	to layout_out.m.
> 
> 	Make tabling pointer variables their natural type.

Don't you need a similar modification to mlds_to_c.m?
The MLDS back-end supports `pragma memo' and `pragma loopcheck'.

> trace/mercury_trace_util.[ch]:
> 	Add a new function MR_trace_is_integer that reads in signed integers.

This function doesn't work for MININT.
Also it doesn't check for overflow.
At very least these flaws should be documented in XXX comments.

> Index: runtime/mercury_tabling.c
...
> @@ -413,6 +433,27 @@
> +#define equal_keys(k1, k2)      (k1 == k2)

s/k1/(k1)/
s/k2/(k2)/

Likewise elsewhere in this file.

> +++ trace/mercury_trace_internal.c	6 Nov 2002 14:56:09 -0000
> @@ -15,6 +15,7 @@
>  #include "mercury_array_macros.h"
>  #include "mercury_getopt.h"
>  #include "mercury_signal.h"
> +#include "mercury_builtin_types.h"
>  
>  #include "mercury_trace.h"
>  #include "mercury_trace_internal.h"
> @@ -257,6 +258,67 @@
>  	const MR_Make_Completer		MR_cmd_arg_completer;
>  } MR_Trace_Command_Info;
>  
> +typedef	struct {
> +	MR_Integer			*MR_ctai_values;
> +	int				MR_ctai_value_next;
> +	int				MR_ctai_cur_index;
> +	MR_Integer			MR_ctai_cur_value;
> +} MR_Int_Table_Arg_Values;

Some comments here would be helpful.

>  static MR_Next
> +MR_trace_cmd_table(char **words, int word_count,
> +	MR_Trace_Cmd_Info *cmd, MR_Event_Info *event_info,
> +	MR_Event_Details *event_details, MR_Code **jumpaddr)
> +{
...
> +	cur_arg = word_count;
> +	num_tips = 0;
> +	for (;;) {
> +		MR_bool	goto_prev_arg;
> +
> +		switch (call_table_args[cur_arg].MR_cta_step) {
> +			case MR_TABLE_STEP_INT:
> +				goto_prev_arg = MR_update_int_table_arg_slot(
> +					&table_cur, &call_table_args[cur_arg]);
> +				break;
> +
> +			case MR_TABLE_STEP_FLOAT:
> +				goto_prev_arg = MR_update_float_table_arg_slot(
> +					&table_cur, &call_table_args[cur_arg]);
> +				break;
> +
> +			case MR_TABLE_STEP_STRING:
> +				goto_prev_arg = MR_update_string_table_arg_slot(
> +					&table_cur, &call_table_args[cur_arg]);
> +				break;
> +
> +
> +			default:
> +				MR_fatal_error("arg not int, float or string "
> +					"after check");
> +		}
> +
> +		if (goto_prev_arg) {
> +			goto prev_arg;
> +		}
> +
> +		cur_arg++;
> +
> +		if (cur_arg >= num_inputs) {
> +			MR_trace_cmd_table_print_tip(proc, num_inputs,
> +				call_table_args, table_cur);
> +			num_tips++;
> +			goto prev_arg_last;
> +		}
> +
> +		continue;
> +
> +	prev_arg:
> +		call_table_args[cur_arg].MR_cta_valid = MR_FALSE;
> +		/* fall through */
> +
> +	prev_arg_last:
> +		cur_arg--;
> +		table_cur = call_table_args[cur_arg].MR_cta_start_node;
> +
> +		if (cur_arg < word_count) {
> +			break;
> +		}
> +	}

This coding style -- with use of gotos, fall-through, and "continue" --
is not the easiest to grok.  Please consider rewriting it so that it
has a less convoluted control flow.

Also, this function is very long.  Consider breaking it into sub-routines.

[to be continued.]

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------