[m-dev.] diff: Accurate GC.

[Fergus Henderson]

On 16-Jul-1998, Tyson Dowd <trd at cs.mu.OZ.AU> wrote:
> +void
> +MR_schedule_agc(Code *pc_at_signal, Word *sp_at_signal)
> +{
...
> +		fprintf(stderr, "Garbage collection scheduled while "
> +				"collector is already running\n");
> +		fprintf(stderr, "Trying to continue...\n");

Those messages should probably be prefixed with
"Mercury runtime: ".  It may be worth defining an
MR_fprintf() that does this.

> +	/* Search for the entry label */
> +
> +#if 0
> +	label = lookup_label_addr(pc_at_signal);
> +	while (label == NULL || label->e_layout == NULL) {
> +		/* 
> +		** Linear search through the label table (should be
> +		** replaced with a binary search through a different
> +		** table).
> +		*/
> +		pc_at_signal = (Code *) ((Word) pc_at_signal - 1);
> +		label = lookup_label_addr(pc_at_signal);
> +	}
> +#endif
> +	entry_label = MR_prev_entry_by_addr(pc_at_signal);
> +	entry_layout = entry_label->e_layout;
> +
> +#if 0
> +	if (label->e_entry == FALSE) {
> +		entry_layout = layout->MR_sll_entry;
> +	} else {
> +		entry_layout = (MR_Stack_Layout_Entry *) layout;
> +	}
> +#endif

Any reason not to just delete the stuff inside `#if 0'?

> +void
> +MR_agc_add_root(Word *root_addr, Word *type_info)
> +{
> +	MR_RootList node;
> +
> +	node = checked_malloc(sizeof(*node));
> +	node->root = root_addr;
> +	node->type_info = type_info;
> +
> +	if (root_list == NULL) {
> +		root_list = node;
> +		last_root = node;
> +	} else {
> +		last_root->next = node;
> +		last_root = node;
> +	}
> +}

You never set node->next for the last node...

> +void
> +MR_agc_dump_roots(MR_RootList roots)
> +{
> +	fflush(NULL);
> +	fprintf(stderr, "Dumping roots\n");
> +
> +	while (roots != NULL) {
> +
> +#ifdef MR_DEBUG_AGC_PRINT_VARS
...
> +#endif
> +		roots = roots->next;
> +	}
> +}

... and yet here you loop until NULL.

Also, shouldn't the `#ifdef' be around the whole
body of the function, or at least around the while loop,
not just the bit that does stuff?

> +extern	void	MR_agc_dump_stack_frames(MR_Internal *label, MemoryZone
> +		*heap_zone, Word * stack_pointer, Word *current_frame);

s/* stack_pointer/*stack_pointer/

> +++ mercury_debug.h	1998/06/30 05:39:44
> @@ -31,6 +31,17 @@
>  
>  #endif
>  
> +	/*
> +	** Define these variables to turn on more debugging information
> +	** for accurate garbage collection.
> +	*/
> +#if MR_DEBUG_AGC
> +  #define MR_DEBUG_AGC_SCHEDULING
> +  #define MR_DEBUG_AGC_COLLECTION
> +  #define MR_DEBUG_AGC_FORWARDING
> +  #define MR_DEBUG_AGC_PRINT_VARS
> +#endif

This should go in the appropriate part of mercury_conf_param.h.  The
idea is that all definitions of configuration macros should be either
on the command line, in mercury_conf.h, or in mercury_conf_param.h.
The reason for this is that if they are scattered around everywhere
then it is difficult to verify that every piece of code that uses a
macro has #included the appropriate header which conditionally defined
that macro.  And if you make a mistake, cpp won't warn you :-(
Instead, the #ifdef will just silently fail.

[C sucks.]

> +#undef  in_range
> +#define in_range(X)	(lower_limit == NULL || ((X) >= lower_limit &&	\
> +				(X) <= upper_limit))

I would wrap that differently:

#define in_range(X)	(lower_limit == NULL || \
			 ((X) >= lower_limit && (X) <= upper_limit))

This makes the semantics a little clearer, IMHO.

> +#ifdef MR_DEBUG_AGC_FORWARDING
> +  #define FORWARD_DEBUG_MSG(Msg, Data)	\
> +		fprintf(stderr, Msg, Data);

Delete the `;' here.

Why is the FORWARD_DEBUG() macro uppercase?

> +                if (in_range(data_value)) {
> +                    /*
> +                    ** force a deep copy by converting to float
> +                    ** and back
> +                    */
> +                    new_data = float_to_word(word_to_float(data));
> +                    leave_forwarding_pointer(data_ptr, new_data);

That isn't going to work, since it will increment hp instead of saved_hp.

> +		if ((entry_array = realloc(entry_array,
> +				entry_array_size * sizeof(MR_Entry)))
>  				== NULL) {
>  			fatal_error("run out of memory for entry label array");
>  		}

That's ugly.  If it won't fit on one line, then for goodness sake
don't try to fit everything in one expression.  Write it as

		entry_array = realloc(entry_array,
				entry_array_size * sizeof(MR_Entry))
		if (entry_array == NULL) {
  			fatal_error("run out of memory for entry label array");
  		}


> +++ mercury_memory.h	1998/07/03 04:38:11
> @@ -105,5 +105,16 @@
>  extern size_t          unit;
>  extern size_t          page_size;
>  
> +/*
> +** Users need to call MR_add_root() for any global variable which
> +** contains pointers to the Mercury heap.  This information is only
> +** used for agc grades.
> +*/

Somewhere on your todo list, you should add an entry that
says "add some documentation to the Memory Management section of the
Mercury user's guide".

> +** get_sp_from_context:
> +** 	Given the signal context, return the Mercury register "MR_sp" at
> +** 	the time of the signal, if available.  If it is unavailable,
> +** 	return NULL.
> +**
> +** XXX Only define this function in accurate gc grades for the moment,

s/Only/We only/
otherwise it sounds like a requirement on the caller.

> +static Word *
> +get_sp_from_context(void *the_context)
> +{
> +	Word *sp_at_signal = NULL;
> +#ifdef NATIVE_GC
... 37 lines ...
> +#else
> +	sp_at_signal = (Word *) NULL;
> +#endif/* NATIVE_GC */

A comment /* !NATIVE_GC */ on the #else would be helpful.
The comment on the endif should be !NATIVE_GC.
A space between the #endif and the comment is probably a good idea.

> +	/* Keep this in sync with the actual defintion below */
> +#define MR_real_reg_number_sp MR_real_reg_number_mr1

s/defintion/definitions/

> +++ mercury_thread.c	1998/07/07 11:11:43
> @@ -27,6 +27,8 @@
>  
>  Declare_entry(do_runnext);
>  
> +MR_MAKE_STACK_LAYOUT_ENTRY(do_runnext);

No semicolon, please.

You did that on purpose, I'm sure! ;-)

Otherwise that looks OK.
I'd be happy for you to commit after addressing those changes,
but I'd like to see a relative diff.

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