[m-dev.] For review: implementation of collect for Opium-M

[Fergus Henderson]

On 01-Nov-1999, Erwan Jahier <Erwan.Jahier at irisa.fr> wrote:
> This change implements the `collect/2' command for opium-M. `collect/2'
> collects runtime information from Mercury program executions. It is intended to
> let users easily implement their own monitors with acceptable performances. It
> looks like the `fold/4' meta-predicate, except:
> 
>    (1)	It operates on-the fly on a sequence of events rather than on a
> 	list.
>    (2)	The accumulator is initialized and updated via Mercury predicates 
> 	whose implementation is in a file pass down as the first argument 
> 	of `collect/2'.

s/pass/passed/

(Another difference with foldl/4 is that it stops when the filter returns `no'.
In fact it's a bit more like `std_util__do_while', the only difference is that
it operators on a sequence of debugger events rather than a sequence of solutions.
Or is that going to be committed as a separate change?)

> browser/collect_lib.m:
> 	New module that defines link_collect/7 that is used during a `collect' 
> 	request: it performs the dynamic linking between collect.so and the
> 	current execution.
> 
> 	Also defines ML_display_close_result() which display the result of
> 	ML_DL__close() function.

s/ML_DL__close/ML_DL_close/

> browser/debugger_interface.m:
...
> 	Define a new function get_collecting_variable_type/2 that retrieves
> 	the type of a variable. This type is needed in MR_trace_event_external()
> 	to be able to call MR_make_permanent() on MR_collecting_variable; we
> 	need to do that to ensure that the memory allocated for it won't be 
> 	deallocated on backtracking.

I think that function should not be needed anymore.

> browser/dl.m:
> 	Move the type definition of the type `handle' to be able to use it in
> 	the collect_lib module.

Is that still needed?
Now that you're calling dl__close, rather than calling dlclose() directly,
I think you should not need to export that type.

> trace/mercury_trace_external.c:
> 	Add support to handle the requests `link_collect', `collect' and 
> 	`current_grade'.
> 
> 	Replace the global variable `searching' that was equal to TRUE when 
> 	searching for a forward matching event and FALSE when reading a request, 
> 	by a enum that is equal to `searching' when searching for a forward 
> 	matching event, `reading_request' when reading a request and `collecting'
> 	when processing a `collect' request.

You should add `MR_' prefixes here in the log message.

> +++ collect_lib.m	Tue Nov  2 04:14:40 1999
...
> +%	To use it, users just need to define 4 things in a file, using the
> +%	Mercury syntax:
> +%		1) a `collected_type' which is the type of the collecting 
> +%		   variable that will contain the result of the monitoring
> +%		   activity.
> +%		2) The predicate initialize/1 which initializes this 
> +%		   collecting variable. initialize/1 should have the
> +%		   following declarations:
> +%			:- pred initialize(collected_type).
> +%			:- mode initialize(out) is det.
> +%		3) the predicate filter/3 which updates this collecting 
> +%		   variable at each execution event. filter/3 should have the 
> +%		   following declarations:
> +%			:- pred filter(event, collected_type, collected_type).
> +%			:- mode filter(in, di, uo) is det.
> +%		4) and eventually the mode definition of the second and the 
> +%		   third arguments of filter/3: `acc_in' and `acc_out'. Those
> +%		   mode have `di' and `uo' respectively as default values.
> +%
> +% 	Then, this file is used to generate the Mercury module `collect.m',
> +% 	which is compiled and dynamically linked with the current execution.
> +% 	When a `collect' request is made from the external debugger, a variable
> +% 	of type collected_type is first initialized (with initialize/1) and
> +% 	then updated (with filter/3) for all the events of the remaining
> +% 	execution. When the end of the execution is reached, the last value of
> +% 	the collecting variable is send to the debugger.

The documentation here will need to be modified for the extra bool argument.

> +:- module collect_lib.
...
> +% We need Handle to be able to close the shared object (dlclose) later on.
> +% When the link failed, we output NULL pointers instead of maybe pointers
> +% for performance reasons; indeed, filter will be called at every events

s/events/event/

> Index: browser/debugger_interface.m
...
> +	% responses to collect
> +	%;	collected(collected_type)

I think you should have a comment here explaining why that is commented out.

> +:- pragma export(close(in, out, di, uo), "ML_DL__close").

s/DL__close/DL_close/

> +/*
> +** Variable generated during the dynamic linking that is needed to close
> +** this linking properly.
> +*/
> +
> +static	Word	*handle = NULL;

I think `collect_lib_handle' would be a better name for that variable.

Why is this declared to have type `Word *' rather than `Word'?
Why is it initialized to NULL?

> +	Word	*close_result;
> +
> +	switch(external_debugger_mode) {
> +		case MR_searching:
> +			MR_send_message_to_socket("forward_move_match_not_found");
> +			break;
> +
> +		case MR_collecting:
> +			(*send_collect_result_ptr)(
> +				(Word) MR_collecting_variable, 

That cast to (Word) should not be needed.

> +				(Word) &MR_debugger_socket_out);
> +			#if defined(HAVE_DLFCN_H) && defined(HAVE_DLCLOSE)
> +		       	ML_DL__close((Word) handle, 
> +		       		(Word *) &close_result);
> +		       	ML_display_close_result((Word) close_result);
> +			#endif
> +			break;

s/DL__close/DL_close/

Declare `close_result' to have type `Word', rather than `Word *',
and then those last two casts won't be needed.

The `(Word) handle' cast also looks a bit odd.
Why is handle declared to have type `Word *' rather than `Word'?

Don't those calls need to be inside MR_TRACE_CALL_MERCURY()?

>  Code *
>  MR_trace_event_external(MR_Trace_Cmd_Info *cmd, MR_Event_Info *event_info)
>  {
> -	static bool	searching = FALSE;
>  	static Word	search_data;
> +	static void	(*initialize_ptr)(Word *) = NULL;
> +	static void    	(*filter_ptr)(Integer, Integer, Integer, MR_Trace_Port,
> +				MR_PredFunc, String, String, String, Integer,
> +				Integer, Integer, String, Word, Word *) = NULL;

Why are these pointers initialized to NULL?

The argument types here need to be Word rather than MR_*,
as discussed in my previous review.

> +			case MR_REQUEST_LINK_COLLECT:
> +			  {
> +			        Char	result;
> +				Word	MR_collecting_variable_type;
> +
> +				if (MR_debug_socket) {
> +					fprintf(stderr, "\nMercury runtime: "
> +						"REQUEST_LINK_COLLECT\n");
> +				}
> +				MR_get_object_file_name(debugger_request,
> +					    &MR_object_file_name);
> +				MR_TRACE_CALL_MERCURY(
> +					ML_DI_link_collect(
> +			       		    MR_object_file_name,
> +					    (void *) &filter_ptr,
> +					    (void *) &initialize_ptr,
> +					    (void *) &send_collect_result_ptr,
> +					    (void *) &get_collect_var_type_ptr,

Those should be cast to (Word *) rather than (void *).

> +					    (Word *) &handle,

That cast should be unnecessary; instead you should change the
type of `handle'.

collect.in:

> :- type declarated_module_name == string.

s/declarated/declared/

I'd like to see another diff for this -- preferably in both
relative diff and full diff forms.

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