[m-dev.] For review: calling MR_collect_filter() in MR_trace_real()

[Fergus Henderson]

On 05-Mar-2001, Erwan Jahier <Erwan.Jahier at irisa.fr> wrote:
> Handling collect requests in MR_trace_real() rather than in
> MR_trace_event_external() as it saves a few indirections and a few
> function calls. It matters since this code is typically called
> several million of times per seconds. 
> 
> trace/mercury_trace_external.c:
> trace/mercury_trace.ch:

s/ch/[ch]/

> 	Handling collect requests in MR_trace_real() rather than in
> 	MR_trace_event_external(). In particular, suppress the 
> 	external_debugger_mode `MR_collecting' in mercury_trace_external.c 
> 	and add the MR_trace_cmd mode `MR_CMD_COLLECT' in mercury_trace.c.
> 
> trace/mercury_trace_external.h:
> 	Make MR_collect_filter(), MR_send_collect_result and
> 	MR_send_message_to_socket() extern functions to be able to use them 
> 	from mercury_trace.c.

I like the general direction of this change.  Handling collect in
MR_trace_real() is much better.

However, I would like to keep the implementation details of the external
tracer, in particular the calls to do socket I/O, out of mercury_trace.h
and mercury_trace.c.

So how about making the following changes:

> Index: trace/mercury_trace.c
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/trace/mercury_trace.c,v
> retrieving revision 1.39
> diff -u -d -u -r1.39 mercury_trace.c
> --- trace/mercury_trace.c	2001/03/05 03:52:29	1.39
> +++ trace/mercury_trace.c	2001/03/05 17:30:14
> @@ -166,6 +166,50 @@
>  #endif	/* MR_TRACE_HISTOGRAM */
>  
>  	switch (MR_trace_ctrl.MR_trace_cmd) {
> +		case MR_CMD_COLLECT:
> +		  {
> +		        MR_Event_Info	event_info;
> +			Word		*saved_regs = event_info.MR_saved_regs;
> +			int		max_r_num;
> +			const char	*path;
> +			bool    	stop_collecting = FALSE;
> +
> +
> +			max_r_num = layout->MR_sll_entry->MR_sle_max_r_num;
> +			if (max_r_num + MR_NUM_SPECIAL_REG > 
> +					MR_MAX_SPECIAL_REG_MR) 
> +			{
> +				event_info.MR_max_mr_num = 
> +					max_r_num + MR_NUM_SPECIAL_REG;
> +			} else {
> +				event_info.MR_max_mr_num = 
> +					MR_MAX_SPECIAL_REG_MR;
> +			}
> +			
> +			port = (MR_Trace_Port) layout->MR_sll_port;
> +			path = layout->MR_sll_entry->MR_sle_module_layout
> +				->MR_ml_string_table + layout->MR_sll_goal_path;
> +			MR_copy_regs_to_saved_regs(event_info.MR_max_mr_num, 
> +				saved_regs);
> +			MR_trace_init_point_vars(layout, saved_regs, port);
> +			MR_COLLECT_filter(seqno, depth, port, layout, 
> +				path, &stop_collecting);
> +			if (stop_collecting) {
> +				MR_send_collect_result();
> +				MR_send_message_to_socket(
> +						"execution_continuing");

I think it would be better to do those three lines above
from inside MR_trace_external().

> +				MR_trace_ctrl.MR_trace_cmd = MR_CMD_GOTO;
> +				MR_copy_saved_regs_to_regs(event_info.MR_max_mr_num, 
> +						saved_regs);
> +				return MR_trace_event(&MR_trace_ctrl, TRUE,
> +                                               layout, port, seqno, depth);
> +			}
> +			MR_copy_saved_regs_to_regs(event_info.MR_max_mr_num, 
> +				saved_regs);

The call to MR_copy_saved_regs_to_regs() is duplicated
in both the `stop_collecting' and the `!stop_collecting'
cases; can it be moved to before the `if (stop_collecting)'?

> Index: trace/mercury_trace_external.c
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/trace/mercury_trace_external.c,v
> retrieving revision 1.53
> diff -u -d -u -r1.53 mercury_trace_external.c
> --- trace/mercury_trace_external.c	2001/01/18 01:19:15	1.53
> +++ trace/mercury_trace_external.c	2001/03/05 17:30:14
> @@ -111,10 +111,9 @@
>  ** (1) `MR_searching', it tries to find an event that matches a forward 
>  **      move request,
>  ** (2) `MR_reading_request', it reads a new request on the socket,
> -** (3) `MR_collecting', it is collecting information (after a `collect' request).
>  */
>  typedef enum {
> -	MR_searching, MR_reading_request, MR_collecting
> +	MR_searching, MR_reading_request
>  } MR_external_debugger_mode_type;

If you leave that alone...

> @@ -457,7 +455,7 @@
>  			MR_send_message_to_socket("forward_move_match_not_found");
>  			break;
>  
> -		case MR_collecting:
> +		case MR_reading_request:
>  			MR_send_collect_result();
>  			MR_send_message_to_socket("execution_terminated");
>  			break;

... and leave that alone...

> @@ -543,18 +535,6 @@
>  			}
>  			break;
>  
> -		case MR_collecting:
> -		 
> -			MR_COLLECT_filter(*filter_ptr, seqno, depth, port,
> -				layout, path, &stop_collecting);
> -
> -			if (stop_collecting) {
> -				MR_send_collect_result();
> -				MR_send_message_to_socket("execution_continuing");
> -				break;
> -			} else {
> -				goto done;
> -			}

... and change that code so that it just contains the
call to MR_send_collect_result() and MR_send_message_to_socket(),
rather than deleting it ...

> @@ -816,9 +796,7 @@
>  						"REQUEST_COLLECT\n");
>  				}
>  				if (collect_linked) {
>  					MR_send_message_to_socket("collect_linked");
> -					external_debugger_mode = MR_collecting;

... and don't delete that line ...

> @@ -1072,7 +1060,7 @@
>  	MR_line_number(MR_debugger_socket_out)++;
>  }
>  
> -static void
> +void
>  MR_send_message_to_socket(const char *message)
>  {
>  	fprintf(MR_file(MR_debugger_socket_out), "%s.\n", message);
...
> @@ -1520,11 +1505,11 @@
>  	*stop_collecting = (result == 'y');
>  }
>  
> -static void
> +void
>  MR_send_collect_result(void)
>  {
>  	MR_TRACE_CALL_MERCURY(

... and leave those two static ...

> Index: trace/mercury_trace_external.h
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/trace/mercury_trace_external.h,v
> retrieving revision 1.9
> diff -u -d -u -r1.9 mercury_trace_external.h
> --- trace/mercury_trace_external.h	2001/02/13 08:28:27	1.9
> +++ trace/mercury_trace_external.h	2001/03/05 17:30:14
> @@ -14,10 +14,16 @@
>  
>  #ifdef	MR_USE_EXTERNAL_DEBUGGER
>  
> +extern  void	MR_send_message_to_socket(const char *message);
>  extern	void	MR_trace_init_external(void);
>  extern	void	MR_trace_final_external(void);
>  extern	MR_Code	*MR_trace_event_external(MR_Trace_Cmd_Info *cmd,
>  			MR_Event_Info *event_info);
> +extern void	MR_COLLECT_filter(MR_Unsigned seqno, 
> +			MR_Unsigned depth, MR_Trace_Port port, 
> +			const MR_Label_Layout *layout, const char *path, 
> +			bool *stop_collecting);
> +extern void	MR_send_collect_result(void);

... and don't add MR_send_collect_result()
and MR_send_message_to_socket() to the header file ...

then I *think* it should all work.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
                                    |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- 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
--------------------------------------------------------------------------