[m-dev.] for review: direct retry

[Fergus Henderson]

On 10-Oct-2000, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Index: compiler/table_gen.m
...
> @@ -613,6 +617,9 @@
>  		[], Module, Context, ResumeGoal0),
>  	append_fail(ResumeGoal0, ResumeGoal1),
>  
> +	true_goal(TrueGoal),
> +	fail_goal(FailGoal),
> +
>  	map__init(StoreMap),
>  	(
>  		EvalMethod = eval_memo
> @@ -622,7 +629,6 @@
>  	;
>  		EvalMethod = eval_loop_check
>  	->
> -		true_goal(TrueGoal),
>  		SaveAnsGoal = TrueGoal,
>  		ActiveGoal = LoopErrorGoal
>  	;
...
> @@ -650,7 +656,6 @@
>  	->
>  		ResumeGoal = ResumeGoal1
>  	;
> -		fail_goal(FailGoal),
>  		ResumeGoal = FailGoal
>  	),
>  	GenAnsGoalEx = disj([GenAnsGoalPart1, ResumeGoal], StoreMap),

That change was not in the log message.
It looks to me like it will make efficiency worse,
so it doesn't seem like a good idea.

> +++ doc/user_guide.texi	2000/10/04 05:39:24
> + at c @sp 1
> + at c @item fail [-NSans] [@var{num}]
> + at c Continues execution until it reaches a FAIL or EXCP port
> + at c of the @var{num}'th ancestor of the call to which the current event refers.
...

It would be a good idea to document why the documentation for `fail'
is commented out.

> Index: runtime/mercury_regorder.h
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/runtime/mercury_regorder.h,v
> retrieving revision 1.14
> diff -u -r1.14 mercury_regorder.h
> --- runtime/mercury_regorder.h	2000/08/03 06:18:52	1.14
> +++ runtime/mercury_regorder.h	2000/10/05 02:08:02
> @@ -152,6 +152,11 @@
>  #define MR_saved_cut_stack(save_area)	LVALUE_CAST(			      \
>  					struct MR_CutStackFrameStruct *,      \
>  					save_area[45])
> +#define MR_saved_r1(save_area)		LVALUE_CAST(MR_Word, save_area[3])
> +#define MR_saved_r2(save_area)		LVALUE_CAST(MR_Word, save_area[4])
> +#define MR_saved_r3(save_area)		LVALUE_CAST(MR_Word, save_area[5])
> +#define MR_saved_r4(save_area)		LVALUE_CAST(MR_Word, save_area[7])
> +#define MR_saved_r5(save_area)		LVALUE_CAST(MR_Word, save_area[8])

How does `MR_saved_r1(save_area)' differ from `saved_reg(save_area, 1)',
where saved_reg() is the existing macro defined in mercury_regs.h?
Other than by having an `MR_' prefix, of course ;-)

> +++ runtime/mercury_stack_layout.h	2000/10/04 13:49:57
> @@ -338,6 +338,13 @@
>  	MR_Stack_Layout_Compiler_Proc	MR_proc_comp;
>  } MR_Stack_Layout_Proc_Id;
>  
> +typedef	enum {
> +	MR_EVAL_METHOD_NORMAL,
> +	MR_EVAL_METHOD_LOOP_CHECK,
> +	MR_EVAL_METHOD_MEMO,
> +	MR_EVAL_METHOD_MINIMAL
> +} MR_EvalMethod;
> +
>  typedef	struct MR_Stack_Layout_Entry_Struct {
>  	/* stack traversal group */
>  	MR_Code			*MR_sle_code_addr;
> @@ -359,6 +366,9 @@
>  	MR_int_least16_t	MR_sle_max_r_num;
>  	MR_int_least8_t		MR_sle_maybe_from_full;
>  	MR_int_least8_t		MR_sle_maybe_trail;
> +	MR_int_least8_t		MR_sle_maybe_maxfr;
> +	MR_EvalMethod		MR_sle_eval_method:8;

Standard ANSI/ISO C does not allow enum bitfields.

> Index: tests/debugger/retry.m
> +% Note: Don't try to print the call stack from within solutions in the input
> +% script, since the results will depend on std_util.m was compiled with
> +% debugging or not.

s/on std_util.m/on whether std_util.m/

> +/*
> +** This function figures out the state of the stacks (i.e. the values of sp,
> +** curfr and maxfr) just after entry to the procedure specified by the given
> +** ancestor level, and returns the proc layout for the specified procedure.
> +** It also

That comment is incomplete.

> +** If it finds that it cannot do its job, it returns NULL and sets *problem
> +** to point to a string giving the reason for its failure.
> +*/
> +
> +static const MR_Stack_Layout_Label *
> +MR_unwind_stacks_for_retry(const MR_Stack_Layout_Label *top_layout,
> +	int ancestor_level, MR_Word **sp_ptr, MR_Word **curfr_ptr,
> +	MR_Word **maxfr_ptr, const char **problem)
> +{
> +	MR_Stack_Walk_Step_Result       result;
> +	const MR_Stack_Layout_Entry	*level_layout;
> +	const MR_Stack_Layout_Label	*return_label_layout;
> +	int				i;
> +
> +	if (ancestor_level < 0) {
> +		*problem = "no such stack frame";
> +		return NULL;
> +	}
> +
> +	return_label_layout = top_layout;
> +	level_layout = top_layout->MR_sll_entry;
> +	*problem = MR_undo_updates_of_maxfr(level_layout,
> +			*sp_ptr, *curfr_ptr, maxfr_ptr);
> +
> +	if (*problem != NULL) {
> +		return NULL;
> +	}
> +
> +	MR_maybe_record_call_table(level_layout, *sp_ptr, *curfr_ptr);
> +
> +	for (i = 0; i < ancestor_level; i++) {
> +		result = MR_stack_walk_step(level_layout, &return_label_layout,
> +				sp_ptr, curfr_ptr, problem);
> +		if (result != STEP_OK) {
> +			if (*problem == NULL) {
> +				*problem = "not that many ancestors";
> +			}
> +
> +			return NULL;

What happens if you're debugging some code, and you type `retry 999'
and get that error message, and then you try to continue debugging?
Since it's already undone the updates of maxfr, couldn't that lead to a crash?

> +static void
> +MR_maybe_record_call_table(const MR_Stack_Layout_Entry *level_layout,
> +	MR_Word *sp, MR_Word *curfr)
> +{
> +	MR_TrieNode	call_table;
> +
> +	if (! MR_ENTRY_LAYOUT_HAS_EXEC_TRACE(level_layout)) {
> +		/*
> +		** The exec trace seems to have disappeared since the call
> +		** to MR_undo_updates_of_maxfr ...
> +		*/
> +
> +		fatal_error("proc layout without exec trace "
> +				"in MR_maybe_record_call_table");

s/fatal_error/MR_fatal_error/

> +		fatal_error("unrecognized retry result");

Likewise.

> +void
> +MR_print_r_regs(FILE *fp, MR_Word *saved_regs)
> +{
> +	fprintf(fp, "r1 = %d (%x)\n",
> +			MR_saved_r1(saved_regs), MR_saved_r1(saved_regs));
> +	fprintf(fp, "r2 = %d (%x)\n",
> +			MR_saved_r2(saved_regs), MR_saved_r2(saved_regs));
> +	fprintf(fp, "r3 = %d (%x)\n",
> +			MR_saved_r3(saved_regs), MR_saved_r3(saved_regs));
> +	fprintf(fp, "r4 = %d (%x)\n",
> +			MR_saved_r4(saved_regs), MR_saved_r4(saved_regs));
> +	fprintf(fp, "r5 = %d (%x)\n",
> +			MR_saved_r5(saved_regs), MR_saved_r5(saved_regs));

That should use the (MR_)saved_reg() macro rather than using
MR_saved_r1(), MR_saved_r2, etc.

Apart from those issues, the rest of this 5000+ line diff 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.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------