[m-rev.] for review: I/O actions in the declarative debugger

[Mark Brown]

On 13-May-2002, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Make I/O actions known to the declarative debugger. The debugger doesn't do
> anything with them yet beyond asking about their correctness.
> 
> browser/io_action.m:
> 	New module for representing I/O actions, and for constructing the map
> 	from I/O action numbers to the actions themselves.
> 
> browser/mdb.m:
> 	Include the new module.
> 
> browser/declarative_analysis.m:
> 	Make the map from I/O action numbers to the actions themselves part
> 	of the analyzer state, since conversions from annotated trace nodes
> 	to EDT nodes may now require this information.

It would be a good idea to document here why the analyser state is chosen
to contain this information, and not, for example, the diagnoser state or
the trace node store.

> trace/mercury_trace_declarative.c:
> 	When invoking the front end, pass to it the boundaries of the required
> 	I/O action map. Cache these boundaries, so we can tell the front end
> 	when reinvocation of the back end (to materialize previously virtual
> 	parts of the annotated trace) do not require the reconstruction of the
> 	map.

The last sentence doesn't make sense.

> Index: browser/declarative_debugger.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/browser/declarative_debugger.m,v
> retrieving revision 1.26
> diff -u -b -r1.26 declarative_debugger.m
> --- browser/declarative_debugger.m	30 Apr 2002 07:08:00 -0000	1.26
> +++ browser/declarative_debugger.m	12 May 2002 09:45:12 -0000
> @@ -321,21 +345,32 @@
>  		"MR_DD_decl_diagnosis_state_init").
>  
>  diagnoser_state_init_store(InStr, OutStr, Diagnoser) :-
> -	diagnoser_state_init(InStr, OutStr, Diagnoser).
> +	diagnoser_state_init(map__init, InStr, OutStr, Diagnoser).
>  
>  	% Export a monomorphic version of diagnosis/9, to make it
>  	% easier to call from C code.
>  	%
>  :- pred diagnosis_store(trace_node_store::in, trace_node_id::in,
> -	diagnoser_response::out, diagnoser_state(trace_node_id)::in,
> +	int::in, int::in, int::in, diagnoser_response::out,
> +	diagnoser_state(trace_node_id)::in,
>  	diagnoser_state(trace_node_id)::out, io__state::di, io__state::uo)
>  	is cc_multi.
>  
> -:- pragma export(diagnosis_store(in, in, out, in, out, di, uo),
> +:- pragma export(diagnosis_store(in, in, in, in, in, out, in, out, di, uo),
>  		"MR_DD_decl_diagnosis").
>  
> -diagnosis_store(Store, Node, Response, State0, State) -->
> -	diagnosis(Store, Node, Response, State0, State).
> +diagnosis_store(Store, Node, UseOldIoActionMap, IoActionStart, IoActionEnd,
> +		Response, State0, State) -->
> +	( { UseOldIoActionMap > 0 } ->
> +		{ State1 = State0 }
> +	;
> +		make_io_action_map(IoActionStart, IoActionEnd, IoActionMap),
> +		{ Analyser0 = State0 ^ analyser_state },
> +		{ analyser_state_replace_io_map(IoActionMap,
> +			Analyser0, Analyser1) },
> +		{ State1 = State0 ^ analyser_state := Analyser1 }
> +	),
> +	diagnosis(Store, Node, Response, State1, State).

The comment for diagnosis_store is no longer accurate because the
implementation does some processing before calling diagnosis.  I suggest
you fix this by moving the new code into diagnosis.

> Index: browser/declarative_oracle.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/browser/declarative_oracle.m,v
> retrieving revision 1.13
> diff -u -b -r1.13 declarative_oracle.m
> --- browser/declarative_oracle.m	2 May 2002 07:44:02 -0000	1.13
> +++ browser/declarative_oracle.m	7 May 2002 09:43:10 -0000
> @@ -191,45 +193,49 @@
>  	tree234_cc__init(N),
>  	tree234_cc__init(X).
>  
> -:- pred get_kb_ground_map(oracle_kb, map_cc(decl_atom, decl_truth)).
> +:- pred get_kb_ground_map(oracle_kb, map_cc(final_decl_atom, decl_truth)).
>  :- mode get_kb_ground_map(in, out) is det.
>  
>  get_kb_ground_map(oracle_kb(Map, _, _, _), Map).
>  
> -:- pred set_kb_ground_map(oracle_kb, map_cc(decl_atom, decl_truth), oracle_kb).
> +:- pred set_kb_ground_map(oracle_kb, map_cc(final_decl_atom, decl_truth),
> +	oracle_kb).
>  :- mode set_kb_ground_map(in, in, out) is det.
>  
>  set_kb_ground_map(oracle_kb(_, Y, N, X), G, oracle_kb(G, Y, N, X)).
>  
> -:- pred get_kb_complete_map(oracle_kb, map_cc(decl_atom, set(decl_atom))).
> +:- pred get_kb_complete_map(oracle_kb,
> +	map_cc(init_decl_atom, set(final_decl_atom))).

You could use the new type, final_decl_atoms, instead of set(final_decl_atom),
both here and below this point.

> Index: browser/declarative_user.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/browser/declarative_user.m,v
> retrieving revision 1.19
> diff -u -b -r1.19 declarative_user.m
> --- browser/declarative_user.m	6 May 2002 08:01:47 -0000	1.19
> +++ browser/declarative_user.m	9 May 2002 06:27:36 -0000
>  
> -:- pred write_decl_atom_limited(user_state::in, decl_atom::in,
> +:- pred write_decl_atom_limited(user_state::in, some_decl_atom::in,
>  	which_headvars::in, io__state::di, io__state::uo) is cc_multi.
>  
> -write_decl_atom_limited(User, atom(PredOrFunc, Functor, Args0), Which) -->
> +write_decl_atom_limited(User, DeclAtom, Which) -->
> +	{ unravel_decl_atom(DeclAtom, TraceAtom, IoActions) },
> +	{ TraceAtom = atom(PredOrFunc, Functor, Args0) },
>  	write_decl_atom_category(User ^ outstr, PredOrFunc),
>  	io__write_string(User ^ outstr, Functor),
>  	io__nl(User ^ outstr),
>  	{ maybe_filter_headvars(Which, Args0, Args) },
> -	foldl(print_decl_atom_arg(User), Args).
> +	list__foldl(print_decl_atom_arg(User), Args),
> +	{ list__length(IoActions, NumIoActions) },
> +	( { NumIoActions = 0 } ->
> +		[]
> +	;
> +		( { NumIoActions = 1 } ->
> +			io__write_string(User ^ outstr, "1 io action:")
> +		;
> +			io__write_int(User ^ outstr, NumIoActions),
> +			io__write_string(User ^ outstr, " io actions:")
> +		),
> + 		( { NumIoActions < 6 } ->
> +			io__nl(User ^ outstr),
> +			list__foldl(print_io_action(User), IoActions)
> +		;
> +			io__write_string(User ^ outstr, " too many to show"),
> +			io__nl(User ^ outstr)
> +		)

Why choose 6 as the limit?  You should make this magic number into a
named constant (that is, a nullary function).  Better still, this parameter
should be made user configurable; however, for this change I'm happy if you
just leave an XXX to this effect.

> Index: trace/mercury_trace_declarative.c
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/trace/mercury_trace_declarative.c,v
> retrieving revision 1.50
> diff -u -b -r1.50 mercury_trace_declarative.c
> --- trace/mercury_trace_declarative.c	9 May 2002 05:23:45 -0000	1.50
> +++ trace/mercury_trace_declarative.c	12 May 2002 13:57:43 -0000
> @@ -1386,8 +1397,27 @@
>  		MR_trace_enabled = MR_TRUE;
>  	}
>  
> +	io_start = MR_edt_start_io_counter;
> +	io_end = MR_io_tabling_counter;
> +

The following code is a bit hard to follow.  It would be clearer if you ...

> +	if (MR_io_action_map_cache_is_valid
> +		&& MR_io_action_map_cache_start <= io_start
> +		&& io_end <= MR_io_action_map_cache_end)
> +	{
> +		use_old_io_map = MR_TRUE;
> +		io_start = MR_io_action_map_cache_start;
> +		io_end   = MR_io_action_map_cache_end;

... delete the above two lines ...

> +	} else {
> +		use_old_io_map = MR_FALSE;
> +
> +		MR_io_action_map_cache_is_valid = MR_TRUE;
> +		MR_io_action_map_cache_start = io_start;
> +		MR_io_action_map_cache_end = io_end;
> +	}
> +
>  	MR_TRACE_CALL_MERCURY(
> -		MR_DD_decl_diagnosis(MR_trace_node_store, root, &response,
> +		MR_DD_decl_diagnosis(MR_trace_node_store, root, use_old_io_map,
> +				io_start, io_end, &response,
>  				MR_trace_front_end_state,
>  				&MR_trace_front_end_state
>  			);

... and pass MR_io_action_map_cache_{start,end} here instead of
io_{start,end}.

Other than that, the change is fine.

Cheers,
Mark.

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