[m-rev.] for review: cleanup mercury_trace_declarative.c

[Julien Fischer]

On Sun, 10 Apr 2005, Ian MacLarty wrote:

> In this diff I have also included compiler generated unifications in the
> annotated trace.  The declarative debugger wont ask questions about these
> because they are automatically trusted, but they are needed in case a subterm
> needs to be tracked though a complicated unification.
>
> I was unable, however to create a test case that caused a __Unify__ predicate
> to be generated which produced output.  Are all compiler generated unifications
> just test unifications?

I'm pretty sure that all of the compiler generated unification
predicates are (in, in). (The code that generates them is in
compiler/unify_proc.m).

>  If not could someone give me an example of a
> complicated unification which produces output and for which the compiler
> generates a seperate procedure?  All the complicated unifications in the
> tests directories seem to be test-unifications (i.e. unifications that produce
> no output).
>
> Cheers,
> Ian.
>
> Estimated hours taken: 5
> Branches: main
>
> Clean up trace/mercury_trace_declarative.c by breaking MR_trace_decl_debug into
> several functions.  This function was getting very big and difficult to
> maintain.
>
> trace/mercury_trace_declarative.c:
> 	Break MR_trace_decl_debug into MR_trace_edt_build_sanity_check, which
> 	checks that we haven't passed the last event for the portion of the
> 	trace being constructed; MR_trace_include_event, which decides if an
> 	event should be included in the annotated trace and resets the depth
> 	if the event is at the top of an area to be materialized;
> 	MR_trace_calculate_event_depth, which calculates the depth in the EDT
> 	of the current event and sets the global MR_edt_depth appropriately;
> 	And MR_trace_construct_node, which constructs a node in the annotated
> 	trace.
>
That would read better as:

	Decompose MR_trace_decl_debug into smaller functions:
	MR_trace_edt_build_sanity_check, ... etc

> 	Uncomment the code that includes compiler generated unifications in the
> 	annotated trace.

The log message should mention why it is ok to uncomment this code now.

>
> Index: trace/mercury_trace_declarative.c
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/trace/mercury_trace_declarative.c,v
> retrieving revision 1.84
> diff -u -r1.84 mercury_trace_declarative.c
> --- trace/mercury_trace_declarative.c	6 Apr 2005 01:11:32 -0000	1.84
> +++ trace/mercury_trace_declarative.c	10 Apr 2005 03:50:08 -0000
> @@ -212,7 +212,9 @@
>  ** was safe to do the first retry across untabled IO.  If they said this was
>  ** okay then there's no point asking them again for the second retry.
>  */
> +
>  static	MR_bool		MR_edt_unsafe_retry_already_asked;
> +
>  /*
>  ** This is used as the abstract map from node identifiers to nodes
>  ** in the data structure passed to the front end.  It should be
> @@ -313,10 +315,6 @@
>  				MR_Trace_Cmd_Info *cmd,
>  				MR_Event_Info *event_info,
>  				MR_Event_Details *event_details);
> -	/*
> -	** Retry max_distance if there are that many ancestors, otherwise
> -	** retry as far as possible.
> -	*/
>  static	MR_Code		*MR_trace_decl_retry_supertree(
>  				MR_Unsigned max_distance,
>  				MR_Event_Info *event_info,
> @@ -331,6 +329,15 @@
>  				MR_Event_Info *event_info);
>  static	void		MR_decl_checkpoint_loc(const char *str,
>  				MR_Trace_Node node);
> +static	void		MR_trace_edt_build_sanity_check(
> +				MR_Event_Info *event_info,
> +				const MR_Proc_Layout *entry);
> +static	MR_bool		MR_trace_include_event(const MR_Proc_Layout *entry,
> +				MR_Event_Info *event_info,
> +				MR_Code **jumpaddr);
> +static	MR_Unsigned	MR_trace_calculate_event_depth(
> +				MR_Event_Info *event_info);
> +static	void		MR_trace_construct_node(MR_Event_Info *event_info);
>
>  MR_bool MR_trace_decl_assume_all_io_is_tabled = MR_FALSE;
>
> @@ -339,6 +346,23 @@
>  MR_bool		MR_trace_decl_in_dd_dd_mode = MR_FALSE;
>
>  /*
> +** We filter out events which are deeper than a certain
Delete the word "certain".

> +** limit given by MR_edt_max_depth.  These events are
> +** implicitly represented in the structure being built.
> +** Adding 1 to the depth limit ensures we get all the
> +** interface events inside a call at the depth limit.
> +** We need these to build the correct contour.
> +** Note that interface events captured at max-depth + 1
Is that max minus depth or max_depth?

> +** will have their at-depth-limit flag set to no.  This
> +** is not a problem, since the parent's flag will be yes,
> +** so there is no chance that the analyser could ask for the
> +** children of an interface event captured at max-depth + 1,
> +** without the annotated trace being rebuilt from the parent
> +** first.
Make that last bit a separate sentence.  ie.

	There is no chance that the analyser could ask ...


> +*/
> +#define MR_TRACE_EVENT_TOO_DEEP(depth) depth > MR_edt_max_depth + 1
> +
You should parantheses around the body of that macro.

> +/*
>  ** This function is called for every traced event when building the
>  ** annotated trace.  It must decide which events are included in the
>  ** annotated trace.
> @@ -348,48 +372,104 @@
>  {
>  	const MR_Proc_Layout 	*entry;
>  	MR_Unsigned		depth;
> -	MR_Trace_Node		trace;
>  	MR_Event_Details	event_details;
>  	MR_Integer		trace_suppress;
> -	MR_Unsigned		depth_check_adjustment = 0;
> +	MR_Unsigned		event_depth;
> +	MR_Code			*jumpaddr;
>
>  	entry = event_info->MR_event_sll->MR_sll_entry;
> -	depth = event_info->MR_call_depth;
>
> +	MR_trace_edt_build_sanity_check(event_info, entry);
> +
> +	if (! MR_trace_include_event(entry, event_info, &jumpaddr)) {
> +		return jumpaddr;
> +	}
> +
> +	event_depth = MR_trace_calculate_event_depth(event_info);
> +
> +	if (MR_TRACE_EVENT_TOO_DEEP(event_depth)) {
> +		return NULL;
> +	}
> +
> +	trace_suppress = entry->MR_sle_module_layout->MR_ml_suppressed_events;
> +	if (trace_suppress != 0) {
> +		/*
> +		** We ignore events from modules that were not compiled
> +		** with the necessary information.  Procedures in those
> +		** modules are effectively assumed correct, so we give
> +		** the user a warning.
> +		*/
That comment is a little unclear, perhaps the following:

	We ignore events from modules that were not compiled with
	the necessary information.  In the absence of such information
	we have to asssume that procedures in those modules are correct.
	We issue a warning to let the user know that this has occurred.

...

> +static	void
> +MR_trace_edt_build_sanity_check(MR_Event_Info *event_info,
> +	const MR_Proc_Layout *entry)
> +{
>  	if (event_info->MR_event_number > MR_edt_last_event
> -			&& !MR_edt_building_supertree) {
> +			&& !MR_edt_building_supertree)
> +	{
>  		/* This shouldn't ever be reached. */
> -		fprintf(MR_mdb_err, "Warning: missed final event.\n");
> +		fprintf(MR_mdb_err, "Error: missed final event.\n");
>  		fprintf(MR_mdb_err, "event %lu\nlast event %lu\n",
>  				(unsigned long) event_info->MR_event_number,
>  				(unsigned long) MR_edt_last_event);
> -		MR_trace_decl_mode = MR_TRACE_INTERACTIVE;
> -		return MR_trace_event_internal(cmd, MR_TRUE, NULL, event_info);
> +		fflush(NULL);
> +		MR_fatal_error("Aborting.");
>  	}
>
>  	if (!MR_PROC_LAYOUT_HAS_EXEC_TRACE(entry)) {
>  		/* XXX this should be handled better. */
>  		MR_fatal_error("layout has no execution tracing");
>  	}
> +}
>
> +static	MR_bool
> +MR_trace_include_event(const MR_Proc_Layout *entry,
> +	MR_Event_Info *event_info, MR_Code **jumpaddr)
> +{
>  	/*
>  	** Filter out events for compiler generated procedures.
> -	** XXX Compiler generated unify procedures should be included
> +	** Compiler generated unify procedures should be included
>  	** in the annotated trace so that sub-term dependencies can be
You should s/sub-term/subterm/ in the line above.

That looks ok otherwise.

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