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

[Fergus Henderson]

On 28-Oct-1999, Erwan Jahier <Erwan.Jahier at irisa.fr> wrote:
> | I'll start off with the easy review comments.
> | (I have one or two more complicated ones brewing... ;-)
> | 
> | On 27-Oct-1999, Erwan Jahier <Erwan.Jahier at irisa.fr> wrote:
> | collect_lib.m:
> | > +%		3) the predicate filter/3 which updates it 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.
> | 
> | It would be better if this predicate also returned a bool
> | to say whether or not to keep on going.
> | The collection process would stop when this predicate returns `no'.
> 
> I will think about that.

OK.

(If you add the bool, then this feature could be used for
emulating breakpoints with user-defined conditions.)

> | > +		%
> | > +		% Look up the address of the first mode (mode number 0)
> | > +		% of the predicates initialize/1 and filter/14  in the 
> | > +		% module collect.
> | > +		%
> | > +		dl__sym(Handle, "Collect_initialize", MaybeInitialize),
> | > +		dl__sym(Handle, "Collect_filter", MaybeFilter),
> | > +		dl__sym(Handle, "Collect_send_collect_result", MaybeSendResult),
> | 
> | The comment here is misleading.  You're actually looking up the
> | address of the corresponding C functions.
> 
> ok.
> I have replaced that comment by:
> 		%
> 		% Look up the address of the initialize/1 and filter/14 
> 		% corresponding C functions in the module collect.
> 		%

I suggest
 		%
 		% Look up the address of the C functions corresponding to the 
		% initialize/1 and filter/14 predicates in the collect module.
 		%

> | In fact a general `link' command to link in an arbitrary shared object
> | might be useful.  You might want to think about whether the
> | parts of this command's functionality which are related specifically
> | to collect could be put in the `collect' command rather than
> | in the `link_collect' command.
> 
> Is it ok if I deal with that in a separate change later on?

Yes.

> | >  Code *
> | >  MR_trace_event_external(MR_Trace_Cmd_Info *cmd, MR_Event_Info *event_info)
> | >  {
> | > -	static bool	searching = FALSE;
> | > +	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 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);
> | > +	static Word    	*handle = NULL;
> | > +	static bool    	collect_linked = FALSE;
> | 
> | The function pointer types look wrong.  For example, why do `initialize_ptr'
> | and `send_collect_result_ptr' point to functions that return values
> | of type `Word', rather than `void'?
> 
> It was to avoid the following warning:
> 
> 	mercury_trace_external.c: In function `MR_trace_event_external':
> 	mercury_trace_external.c:484: warning: passing arg 1 of pointer to function makes pointer 
> 	from integer without a cast
> 	mercury_trace_external.c:484: warning: passing arg 2 of pointer to function makes pointer 
> 	from integer without a cast

That warning is about the type of the first and second arguments,
not about the return type.

When you call the initialize_ptr function (in mercury_trace_external.c),
you call it like so:

|	 (*initialize_ptr)(&collecting_variable));

Here you ignore the function's return value.
So probably the return value should have type `void'.

> And I though it was the type of pointers on Mercury objets.
> So, what should it be?

Have a look at the header file collect.h that the Mercury compiler
generates; it will have declarations for the "Collect_filter",
"Collect_initialize", and "Collect_send_collect_result" functions.
The declarations for the function pointers should exactly match
those automatically-generated function declarations.

Another point, while I think of it: for consistency with the
naming conventions used elsewhere in the Mercury implementation,
the names used for those functions in the `pragma export' declarations
should probably be something like `ML_COLLECT_filter', `ML_COLLECT_initialize',
etc., rather than `Collect_filter', `Collect_initialize', etc.

> | Also, the use of MR_Trace_Port
> | and MR_PredFunc looks dangerous; the type here should exactly match
> | the type with which the function is defined, and I'll bet that it
> | will be defined using Integer or Word rather than MR_Trace_Port.
> 
> Well, collect.m is generated with the help of the file collect.in (which is 
> not typed by users) which contains:
> 
> 	% The stuff defined below is similar to types goal_path and trace_port
> 	% defined in modules compiler/hlds_goal.m and compiler/trace.m.
> 	% This enumeration must be EXACTLY the same as the MR_trace_port enum in
> 	% runtime/mercury_trace_base.h, and in the same order, since the code
> 	% assumes the representation is the same.
>
> 	:- type trace_port_type
> 		--->	call
> 		;	exit
> 		;	redo
> 		;	fail
> 		...
> 
> In runtime/mercury_trace_base.h, there is the comment:
> 
> 	/*
> 	** This enum should EXACTLY match the definition of the `trace_port_type'
> 	** type in browser/debugger_interface, and the port names list in the
> 	** C source file of this module.
> 	*/
> 
> 	typedef	enum {
> 		MR_PORT_CALL,
> 		MR_PORT_EXIT,
> 		...}

Actually I think that code only assumes that the representation of the
enum VALUES are the same.  I don't think it assumes that the representation
of the TYPES are the same.  (If it does, then that assumption is a bug.)
In particular, although the same numerical values will be used, the C
compiler may well choose to use a `short' or `unsigned short' or
even `unsigned char' type to represent the enum.  There is no guarantee
that the type used will have the same representation and calling convention
as `Word' or `Integer'.

In particular, for the `pred_or_func'/`MR_PredFunc' type, the enum has
only two values.  The numerical values of these two will always be the
same (pred = MR_PREDICATE = 0, func = MR_FUNCTION = 1).  But a clever C
compiler might recognize that the MR_PredFunc type has only two values
and thus might try passing values of type MR_PredFunc in a single-bit
condition register rather than in a full-word register.  The calling
convention might depend on the type.

It might be best to change the definitions of the types so that
they are typedefs for `Word' rather than being enums.  The enumeration
constants would of course stay.  So the declarations would look like
this

	enum { MR_PORT_CALL, MR_PORT_EXIT, ... };
	typedef Word MR_Trace_Port;

or perhaps like this

	typedef enum { MR_PORT_CALL, MR_PORT_EXIT, ... } MR_Trace_Port_Enum;
	typedef Word MR_Trace_Port;

> Maybe I should replace it by:
> 
> 	/*
> 	** This enum should EXACTLY match the definition of the `trace_port_type'
> 	** type in browser/debugger_interface and extras/opium_m/source/collect.in, 
> 	** and the port names list in the C source file of this module.
> 	*/
> 
> ??

That would be a good idea too.  But for reasons explained above
it is not sufficient.

> | The indentation of the `case' should be the same as that of the `switch'.
> 
> It was to be consistent with the indentation of the switch that accurs after 
> in the code

I checked the "C coding guidelines" on our WWW page,
and I found that according to that, you are right,
and I am wrong.  So please leave it as is.

I also had a look for switches through our source code, and found that
most of it does follow the style recommended in our C coding guidelines.
There were a few places which did not:

	runtime/mercury_memory_handlers.c
	runtime/mercury_wrapper.c
	library/std_util.m
	runtime/mercury_deep_copy_body.h

Probably we should change those places...

> | > +		case collecting:
> | > +			if (seqno == 1 && port == MR_PORT_EXIT) {
> | > +				/* The end of the execution is reached */
> | 
> | That condition is wrong; it does not necessarily indicate the last
> | event.  For one thing, you should use MR_port_is_final(port) rather
> | than `port == MR_PORT_EXIT', because the final port could be an
> | exception or a fail port.  Even that is not enough, though:  if `main'
> | is compiled without tracing, but some other modules that it calls are
> | compiled with tracing, then you can reach the exit port for event #1
> | long before all the events have finished.
> 
> Yes, you are right.
> 
> I'm now sending the result via MR_trace_final_external() (which is even more
> efficient since I win a switch ;-); this implied to declare
> external_debugger_mode, MR_collecting_variable, send_collect_result_ptr, handle
> as global.
> 
> Do you think it ok?

Yes.

> | Content-Description: collect.op
> | ...
> | > % There are several things to do in order to be able to execute a 
> | > % collect/1 command:
> | > % 1) create a file that will that contain the definition of collected_type, 
> | > %	initialize/1 and filter/3,
> | > % 2) generate `collect.m' from this file (generate_collect/1),
> | > % 3) compile collect.m (compile_collect/0),
> | > % 4) dynamically link it with the current execution (dyn_link_collect/0).
> | > % 5) run the command (run_command/1).
> | 
> | It would be nice if there was a single command that did steps 2-5
> | all at once.
> 
> Why? 
> 
> Note that collect/2 (which does 1-5 all at once)

That is sufficient; I didn't notice the existence of that command.

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