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

[Fergus Henderson]

On 28-Oct-1999, Erwan Jahier <Erwan.Jahier at irisa.fr> wrote:
> 
>  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.
...
> +:- pred get_collecting_variable_type(T, type_info).
> +:- mode get_collecting_variable_type(in, out) is det.
> +
> +:- pragma export(get_collecting_variable_type(in, out), 
> +	"ML_DI_get_collecting_variable_type").
> +	% This predicate allows mercury_trace_external.c to retrieve the name 
> +	% of the type_info a collecting variable.
> +get_collecting_variable_type(CollectVar, Type) :-
> +	Type = type_of(CollectVar).

That predicate is not very useful; since the predicate is declared to
have a polymophic type `T', the exported C function will need to be
passed a type_info argument, and will just return that same type_info.

To make this work, you need the predicate to be declared with a concrete
type, which probably means it needs to be defined in collect.in, I think.

> +	case MR_collecting:
> +		(*send_collect_result_ptr)(
> +			(Word) MR_collecting_variable, 
> +			(Word) &MR_debugger_socket_out);
> +			#if defined(HAVE_DLFCN_H)&&defined(HAVE_DLCLOSE)
> +				dlclose((void *)handle);
> +			#endif

s/&&/ && /

Rather than calling dlclose() directly here, it would be better
to call the Mercury dl__close procedure.  Note that eventually
the dl__close procedure may need to do additional work which is
not done by dlclose(), e.g. removing the module's procedures from
the various tables maintained by the Mercury debugger.
If you call dl__open to open it, then you should call dl__close
to close it.

> @@ -397,16 +450,11 @@
>  Code *
>  MR_trace_event_external(MR_Trace_Cmd_Info *cmd, MR_Event_Info *event_info)
>  {
> -	static MR_external_debugger_mode_type 
> -			external_debugger_mode = reading_request;
>  	static Word	search_data;
> -	static Word	collecting_variable;
> -	static Word	(*initialize_ptr)(Word *) = NULL;
> +	static void	(*initialize_ptr)(Word *) = NULL;
>  	static Word    	(*filter_ptr)(Integer, Integer, Integer, MR_Trace_Port,
>  				MR_PredFunc, String, String, String, Integer,
>  				Integer, Integer, String, Word, Word *) = NULL;
> -	static Word	(*send_collect_result_ptr)(Word, Word);

The return types of the filter_ptr and send_collect_result_ptr are still wrong,
I think -- shouldn't they be `void', not `Word'?

> +		case MR_collecting:
> +			/* 
> +			** XXX Add a another request that takes 
> +			** arguments into account. We need two kinds 
> +			** of request in order to not penalize the 
> +			** performance of collect in the cases where 
> +			** arguments are not used.
> +			**  
> +			** arguments = MR_make_var_list(layout, saved_regs);
> +			*/
> +			MR_COLLECT_filter(
> +				*filter_ptr, 
> +				(Unsigned) seqno, 
> +				(Unsigned) depth, 
> +				(MR_Trace_Port) port,
> +				layout, 
> +				path);
> +			goto done;

The casts here are unnecessary, I think.

>  			case MR_REQUEST_LINK_COLLECT:
>  			  {
>  			        Char	result;
> +				Word	MR_collecting_variable_type;
> +				Word	*MR_collecting_variable_type_info=NULL;

Why do you initialize the type_info to NULL here?

>  				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,
>  					    (Word *) &filter_ptr,
>  					    (Word *) &initialize_ptr,
>  					    (Word *) &send_collect_result_ptr,
>  					    (Word *) &handle,
> -					    (Char *) &result
> +					    &result
>  					    ));
>  				collect_linked = (result == 'y');
>  				if (collect_linked) {
>  					MR_send_message_to_socket(
>  						"link_collect_succeeded");
> +					MR_TRACE_CALL_MERCURY(
> +					    ML_DI_get_collecting_variable_type(
> +						(Word) 
> +						MR_collecting_variable_type_info,
> +						MR_collecting_variable,
> +						(Word *) 
> +						&MR_collecting_variable_type));

The cast to (Word *) here is unnecessary.
In fact the whole call to ML_DI_get_collecting_variable_type
is unnecessary; given the definition of get_collecting_variable_type
that you had, that call will just set MR_collecting_variable_type to
MR_collecting_variable_type_info, so you might as well replace it with

	MR_collecting_variable_type = MR_collecting_variable_type_info;
	
Unfortunately the result will still be a NULL pointer, and so
probably the following call to MR_make_permanent() will crash, if you test
it in a grade which does not use conservative GC.

>  					/*
>  					** In order to perform the collect from
>  					** the current event, we need to call 
>  					** filter once here.
>  					*/
> +					MR_COLLECT_filter(
> +						*filter_ptr, 
> +						(Unsigned) seqno, 
> +						(Unsigned) depth, 
> +						(MR_Trace_Port) port,
> +						layout, 
> +						path);
> +					

The casts there are unnecessary, I think.

> @@ -61,12 +61,12 @@
>  % for performance reasons; indeed, filter will be called at every events
>  % so we don't want to pay the price of the maybe variable de-construction
>  % at each event.
> -link_collect(Filter, Initialize, SendResult, HandlePtr, Result) -->
> +link_collect(ObjetFile, Filter, Initialize, SendResult, HandlePtr, Result) -->

s/ObjetFile/ObjectFile/g

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