[m-dev.] diff: Accurate GC.

[Tyson Dowd]

On 16-Jul-1998, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> 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.

It wouldn't be worth it unless you fix all the other occurances too,
which isn't appropriate for this diff.  So I'll leave MR_fprintf
for another time.

> 
> > +	/* 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'?

Once, yes.  Not anymore.

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

Strange, because it works.  Lucky I guess.
Good spotting -- they should get you to mark 1st year assignments!

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

Yep.

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

Ok, good idea.

> 
> [C sucks.]

This doesn't suck as much as that "no empty compilation units" rule.

> 
> > +#ifdef MR_DEBUG_AGC_FORWARDING
> > +  #define FORWARD_DEBUG_MSG(Msg, Data)	\
> > +		fprintf(stderr, Msg, Data);
> 
> Delete the `;' here.
>

Hmmm... In the #else case, it is defined as nothing.  That will mean
	forward_debug_msg(....);
will become an *evil* lone semicolon.

What do you recommend as "null" code to put in the #else case.
Or I suppose I could just leave all the semicolons off the calls to
"forward_debug_msg" (yeech).

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

Yep, it's actually quite easy to just avoid this hack and do it properly.

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

Yep.

> > +++ 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! ;-)

Just testing...	(actually, I wish it had been on purpose).

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

Here:

diff -N -u runtime2/mercury_accurate_gc.c runtime/mercury_accurate_gc.c
--- runtime2/mercury_accurate_gc.c	Mon Jul 13 16:44:28 1998
+++ runtime/mercury_accurate_gc.c	Tue Jul 21 23:11:28 1998
@@ -80,9 +80,9 @@
 		** garbage has been solved).
 		*/
 
-		fprintf(stderr, "Garbage collection scheduled while "
-				"collector is already running\n");
-		fprintf(stderr, "Trying to continue...\n");
+		fprintf(stderr, "Mercury runtime: Garbage collection scheduled"
+				" while collector is already running\n");
+		fprintf(stderr, "Mercury_runtime: Trying to continue...\n");
 		return;
 	}
 
@@ -96,29 +96,9 @@
 
 	/* 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
-
 	determinism = entry_layout->MR_sle_detism;
 
 	if (determinism < 0) {
@@ -126,9 +106,9 @@
 		** This means we have reached some handwritten code that has
 		** no further information about the stack frame.
 		*/
-		fprintf(stderr, "LABEL NAME: %s has no stack layout info\n",
-			entry_label->e_name);
-		fprintf(stderr, "Trying to continue...\n");
+		fprintf(stderr, "Mercury runtime: LABEL: %s has no stack"
+				"layout info\n", entry_label->e_name);
+		fprintf(stderr, "Mercury runtime: Trying to continue...\n");
 		return;
 	}
 
@@ -486,6 +466,7 @@
 	if (root_list == NULL) {
 		root_list = node;
 		last_root = node;
+		last_root->next = NULL;
 	} else {
 		last_root->next = node;
 		last_root = node;
diff -N -u runtime2/mercury_agc_debug.c runtime/mercury_agc_debug.c
--- runtime2/mercury_agc_debug.c	Mon Jul 13 15:26:02 1998
+++ runtime/mercury_agc_debug.c	Tue Jul 21 23:14:15 1998
@@ -29,9 +29,9 @@
 	fflush(NULL);
 	fprintf(stderr, "Dumping roots\n");
 
+#ifdef MR_DEBUG_AGC_PRINT_VARS
 	while (roots != NULL) {
 
-#ifdef MR_DEBUG_AGC_PRINT_VARS
 
 		/*
 		** Restore the registers, because we need to save them
@@ -52,9 +52,9 @@
 
 		MR_copy_saved_regs_to_regs(MAX_REAL_REG + NUM_SPECIAL_REG);
 		save_registers();
-#endif
 		roots = roots->next;
 	}
+#endif
 }
 
 void
diff -N -u runtime2/mercury_agc_debug.h runtime/mercury_agc_debug.h
--- runtime2/mercury_agc_debug.h	Mon Jul  6 17:08:00 1998
+++ runtime/mercury_agc_debug.h	Tue Jul 21 23:17:08 1998
@@ -24,7 +24,7 @@
 */
 
 extern	void	MR_agc_dump_stack_frames(MR_Internal *label, MemoryZone
-		*heap_zone, Word * stack_pointer, Word *current_frame);
+		*heap_zone, Word *stack_pointer, Word *current_frame);
 
 /*
 ** MR_agc_dump_roots:
diff -N -u runtime2/mercury_conf_param.h runtime/mercury_conf_param.h
--- runtime2/mercury_conf_param.h	Mon Jul 13 16:00:12 1998
+++ runtime/mercury_conf_param.h	Tue Jul 21 23:19:03 1998
@@ -152,7 +152,16 @@
 ** MR_DEBUG_AGC
 ** 	Turn on all debugging information for accurate garbage
 ** 	collection.  (Equivalent to all MR_DEBUG_AGC_* macros above).
-**
+*/
+
+#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
+
+/*
 ** MR_LABEL_STRUCTS_INCLUDE_NUMBER
 **	Include a label number in each label layout structure.
 */
diff -N -u runtime2/mercury_debug.h runtime/mercury_debug.h
--- runtime2/mercury_debug.h	Tue Jun 30 15:39:44 1998
+++ runtime/mercury_debug.h	Tue Jul 21 23:17:59 1998
@@ -31,17 +31,6 @@
 
 #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
-
 #ifndef MR_LOWLEVEL_DEBUG
 
 #define	dump_push_msg(msg)			((void)0)
diff -N -u runtime2/mercury_deep_copy.c runtime/mercury_deep_copy.c
--- runtime2/mercury_deep_copy.c	Thu Jul 16 00:01:54 1998
+++ runtime/mercury_deep_copy.c	Tue Jul 21 23:27:24 1998
@@ -26,8 +26,8 @@
 */
 
 #undef  in_range
-#define in_range(X)	(lower_limit == NULL || ((X) >= lower_limit &&	\
-				(X) <= upper_limit))
+#define in_range(X)	(lower_limit == NULL || \
+				((X) >= lower_limit && (X) <= upper_limit))
 
 #undef	maybeconst
 #define	maybeconst	const
@@ -71,7 +71,7 @@
 
 #ifdef MR_DEBUG_AGC_FORWARDING
   #define FORWARD_DEBUG_MSG(Msg, Data)	\
-		fprintf(stderr, Msg, Data);
+		fprintf(stderr, Msg, Data)
 #else
   #define FORWARD_DEBUG_MSG(Msg, Data)
 #endif
diff -N -u runtime2/mercury_deep_copy_body.h runtime/mercury_deep_copy_body.h
--- runtime2/mercury_deep_copy_body.h	Thu Jul 16 00:02:27 1998
+++ runtime/mercury_deep_copy_body.h	Tue Jul 21 23:51:49 1998
@@ -150,11 +150,8 @@
         case MR_DATAREP_FLOAT:
             #ifdef BOXED_FLOAT
                 if (in_range(data_value)) {
-                    /*
-                    ** force a deep copy by converting to float
-                    ** and back
-                    */
-                    new_data = float_to_word(word_to_float(data));
+                    incr_saved_hp(new_data, FLOAT_WORDS);
+                    field(0, new_data, 0) = *data_value;
                     leave_forwarding_pointer(data_ptr, new_data);
                 } else {
                     new_data = data;
diff -N -u runtime2/mercury_label.c runtime/mercury_label.c
--- runtime2/mercury_label.c	Wed Jul  8 16:37:13 1998
+++ runtime/mercury_label.c	Tue Jul 21 23:57:06 1998
@@ -109,9 +109,9 @@
 
 	if (entry_array_next >= entry_array_size) {
 		entry_array_size *= 2;
-		if ((entry_array = realloc(entry_array,
-				entry_array_size * sizeof(MR_Entry)))
-				== NULL) {
+		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");
 		}
 	}
diff -N -u runtime2/mercury_memory_handlers.c runtime/mercury_memory_handlers.c
--- runtime2/mercury_memory_handlers.c	Wed Jul 15 23:42:05 1998
+++ runtime/mercury_memory_handlers.c	Wed Jul 22 00:00:08 1998
@@ -577,7 +577,7 @@
 ** 	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,
+** XXX We only define this function in accurate gc grades for the moment,
 ** because it's unlikely to compile everywhere.  It relies on
 ** MR_real_reg_number_sp being defined, which is the name/number of the
 ** machine register that is used for MR_sp.
@@ -625,9 +625,9 @@
 	sp_at_signal = (Word *) NULL;
 
   #endif
-#else
+#else /* !NATIVE_GC */
 	sp_at_signal = (Word *) NULL;
-#endif/* NATIVE_GC */
+#endif /* !NATIVE_GC */
 
 	return sp_at_signal;
 }
diff -N -u runtime2/mercury_regorder.h runtime/mercury_regorder.h
--- runtime2/mercury_regorder.h	Wed Jun 24 12:26:15 1998
+++ runtime/mercury_regorder.h	Wed Jul 22 00:01:02 1998
@@ -67,7 +67,7 @@
 #define r31		count_usage(R_RN(31), mr36)
 #define r32		count_usage(R_RN(32), mr37)
 
-	/* Keep this in sync with the actual defintion below */
+	/* Keep this in sync with the actual defintions below */
 #define MR_real_reg_number_sp MR_real_reg_number_mr1
 
 #define MR_engine_base	LVALUE_CAST(Word *, count_usage(MR_SP_RN, mr0))
diff -N -u runtime2/mercury_thread.c runtime/mercury_thread.c
--- runtime2/mercury_thread.c	Tue Jul  7 21:11:43 1998
+++ runtime/mercury_thread.c	Wed Jul 22 00:02:13 1998
@@ -27,7 +27,7 @@
 
 Declare_entry(do_runnext);
 
-MR_MAKE_STACK_LAYOUT_ENTRY(do_runnext);
+MR_MAKE_STACK_LAYOUT_ENTRY(do_runnext)
 
 #ifdef MR_THREAD_SAFE
 MercuryThread *


-- 
       Tyson Dowd           # "Bill Gates is a white persian cat and a monocle
                            # away from becoming another James Bond villan."
     trd at cs.mu.oz.au        # "No Mr Bond, I expect you to upgrade."
http://www.cs.mu.oz.au/~trd #                -- Dennis Miller and Terri Branch