[m-rev.] For review: changes to declarative debugger (Part 1)

[Zoltan Somogyi]

On 29-Sep-2004, Ian MacLarty <maclarty at cs.mu.OZ.AU> wrote:
> For review by Zoltan.

Here is my review of the initial portion. More to come later.

Zoltan.

> Lot's of stuff in declarative_analyser.m has been redesigned to facilitate 
> future improvements, such as probabalistic debugging.

Lot's -> Lots

> browser/program_representation.m
> 	Added builtin_call_rep to represent builtin calls.
> 	Made plain calls to UCI predicates be treated as primitive ops.
> 	Added function to say if a goal generates internal events directly.
> 	Added a function to say whether an atomic goal is identifiable (i.e.
> 	whether we can get from its goal_rep its name, module and arity).

> tests/debugger/declarative/trust.inp2
> 	Tests for new debugger features and changes to some old tests.

What are those changes?

> trace/mercury_trace_declarative.c
> 	Made the depth step size used when deciding which events to put in 
> 	the annotated trace a variable so that it can be dynamically adjusted
> 	in the future.
> 	The EDT depth is now calculated instead of using the call depth (which
> 	is not always consistent with the EDT depth).

I presume you mean "instead of the call depth"?

> 	Made changes so that all the interface events for the sub-calls inside
> 	a call are included in the annotated trace, so that contours are built
> 	correctly.

This breaks the depth limit, so I suspect you don't mean that in all cases.

> :- pred revise_analysis(S::in, analyser_response(T)::out, analyser_state(T)::in,
> 	analyser_state(T)::out) is det <= mercury_edt(S, T).

Break up that line.

> 			% Look for the first unknown suspect in a breadth-first 
> 			% fashion, starting at the root.  If no unknown
> 			% suspects are found then choose a skipped suspect
> 			% to requery.
> 	--->	breadth_first

Breadth first is a bad name. Neither breadth_first nor depth_first mean
much here, because if a node is valid, its children aren't searched.
I think you mean simply top-down left-to-right.

> 			%
> 			% Follow the subterm all the way to where it's bound or
> 			% until it can't be followed any further (for example
> 			% when there is a call to a module with no tracing),
> 			% and ask a question about the nearest unknown suspect
> 			% on the subterm dependency chain.  Then proceed to do
> 			% a binary search between this node and the root of the
> 			% search space (The binary search will only come into
> 			% effect if the oracle asserts the suspect is correct
> 			% or inadmissible).

"The" -> "the"

> 			% explicit subtree to be generated.  The last argument
> 			% is the last suspect on the dependency chain whos
> 			% status was unknown (initially this is no).

"whos" -> "whose"

Was unknown when? Last in what direction? More documentation needed here.

> 			%
> 			% Perform a binary search on the array of suspects.
> 			% The range field gives the subrange of the array to
> 			% search.  last_tested is the index into the array of
> 			% the last suspect about which a question was asked.   
> 			%

Where do the array elements come from, and what invariants hold on them?
The range endpoints: are they inclusive or exclusive?

> 	;	binary(
> 			suspects	:: array(suspect_id),
> 			range		:: pair(int, int),
> 			last_testsed	:: int
> 		).

last_testsed -> last_tested

> analyser_state_init(IoActionMap, Analyser) :-
> 	empty_search_space(EmptySearchSpace),
> 	Analyser = analyser(EmptySearchSpace, [], no, breadth_first, no, 
> 		IoActionMap, no).

Make empty_search_space a function.

> analyser_state_replace_io_map(IoActionMap, Analyser0, Analyser) :-
> 	Analyser = Analyser0 ^ io_action_map := IoActionMap.

Use state variable notation.

> start_analysis(Store, Tree, Response, !Analyser) :-
> 	MaybeRequireExplicit = !.Analyser ^ require_explicit,
> 	(
> 		MaybeRequireExplicit = yes(SuspectId),
> 		incorporate_explicit_subtree(SuspectId, Tree, 
> 			!.Analyser ^ search_space, SearchSpace),
> 		!:Analyser = !.Analyser ^ search_space := SearchSpace,
> 		!:Analyser = !.Analyser ^ require_explicit := no,
> 		decide_analyser_response(Store, Response, !Analyser)
> 	;
> 		MaybeRequireExplicit = no,
> 		%
> 		% An explicit subtree was not requested, so this is the 
> 		% start of a new declarative debugging session.
> 		%
> 		reset_analyser(!Analyser),
> 		initialise_search_space(Tree, SearchSpace),
> 		!:Analyser = !.Analyser ^ search_space := SearchSpace,
> 		root_det(SearchSpace, RootId),
> 		!:Analyser = !.Analyser ^ last_search_question := yes(RootId),
> 		edt_question(!.Analyser ^ io_action_map, Store, Tree, 
> 			Question),
> 		Response = revise(Question)
> 	).

Since start_analysis with MaybeRequireExplicit = yes just continues an
existing analysis, you should rename the predicate.

> revise_analysis(Store, Response, !Analyser) :-
> 	%
> 	% The head of the previous_roots field in the analyser is just the

Fields don't have heads, only lists do.

> 	% current root of the search space, so we make the second element in
> 	% previous_roots the new erroneous root, make everything below it
> 	% unknown and re-query the current root.  If there's only one previous

Is "current root" the same as "new erroneous root"?

> search(Store, !SearchSpace, breadth_first, NewMode, Response) :-
> 	breadth_first_search(Store, !SearchSpace, Response, NewMode).

breadth_first_search should be renamed, for the reason given above.

> breadth_first_search(Store, !SearchSpace, Response, NewMode) :-
> 	root_det(!.SearchSpace, RootId),
> 	(
> 		first_unknown_descendent_breadth(Store, RootId, 
> 			!.SearchSpace, SearchSpace1, MaybeDescendent)
> 	->
> 		SearchSpace1 = !:SearchSpace,
> 		(
> 			MaybeDescendent = yes(Unknown),
> 			Response = question(Unknown),
> 			NewMode = breadth_first
> 		;
> 			MaybeDescendent = no,
> 			(
> 				choose_skipped_suspect(!.SearchSpace,
> 					LeastSkipped)
> 			->
> 				Response = question(LeastSkipped),
> 				NewMode = breadth_first

Why *Least*Skipped?

> 				throw(internal_error("breadth_first_search",
> 					"no unknown or skipped suspects"))
> 			)
> 		)
> 	;
> 		%
> 		% An explicit subtree is required, so pick an implicit root
> 		% to make explicit.
> 		% 
> 		(
> 			pick_implicit_root(Store, !.SearchSpace, ImplicitRoot)
> 		->
> 			Response = require_explicit(ImplicitRoot),
> 			NewMode = breadth_first

Could this decide to materialize a node that is not a child of RootId? If yes,
why? If not, document why it can't.

> 			throw(internal_error("breadth_first_search",
> 				"first_unknown_descendent_breadth required "++
> 				"an explicit subtree, but pick_implicit_roo"++
> 				"t couldn't find an implicit root"))

The error message is contradictory.

> follow_subterm_end_search(Store, !SearchSpace, LastUnknown, SuspectId, ArgPos, 
> 		TermPath, NewMode, SearchResponse) :-
> 	find_subterm_origin(Store, SuspectId, ArgPos, TermPath, !SearchSpace,
> 		FindOriginResponse),
> 	root_det(!.SearchSpace, RootId),
> 	(
> 		FindOriginResponse = primitive_op(_, _),
> 		(
> 			LastUnknown = yes(Unknown),
> 			SearchResponse = question(Unknown),
> 			setup_binary_search(!.SearchSpace, RootId, Unknown,
> 				NewMode)
> 		;
> 			LastUnknown = no,
> 			breadth_first_search(Store, !SearchSpace, 
> 				SearchResponse, NewMode)
> 		)
> 	;
> 		FindOriginResponse = not_found,
> 		(
> 			LastUnknown = yes(Unknown),
> 			SearchResponse = question(Unknown),
> 			setup_binary_search(!.SearchSpace, RootId, Unknown,
> 				NewMode)
> 		;
> 			LastUnknown = no,
> 			breadth_first_search(Store, !SearchSpace,
> 				SearchResponse, NewMode)
> 		)

The code for primitive_op and not_found seem identical. Is this intentional,
and if so, why?

> 		(
> 			suspect_unknown(!.SearchSpace, OriginId)
> 		->
> 			follow_subterm_end_search(Store, !SearchSpace, 
> 				yes(OriginId), OriginId, OriginArgPos, 
> 				OriginTermPath, NewMode, SearchResponse)
> 		;
> 			follow_subterm_end_search(Store, !SearchSpace, 
> 				LastUnknown, OriginId, OriginArgPos,
> 				OriginTermPath, NewMode, SearchResponse)
> 		)
> 	).

Factor out the common code of the call here. Document why the recursive call
won't lead to an infinite loop.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------