[Gc] Fix for GC_general_register_disappearing_link and GC_register_finalizer

Ivan Maidanski ivmai at mail.ru
Wed Feb 11 00:46:27 PST 2009


Hi!

When GC_oom_fn() is used in GC_general_register_disappearing_link() and GC_register_finalizer_inner() as the last chance to get memory, upon success (it's unlikely but...) we should (after gaining lock) recalculate hashtable index (since the table may grow by another thread while we were waiting for lock or, even, by client GC_oom_fn() itself) and check again that our entity (link or finalizer) not present in the hashtable.

The attached patch does this and some more:
- *ocd and *ofn remains unchanged now if GC_register_finalizer_inner() fails (for lack of memory);
- the comment for GC_finalize() is corrected (finalizers aren't invoked here only enqueued);
- the comments for GC_general_register_disappearing_link() and GC_unregister_disappearing_link() are refined (and a typo fixed) in gc.h.

Bye.

-------------- next part --------------
diff -ru bdwgc/finalize.c updated/bdwgc/finalize.c
--- bdwgc/finalize.c	2009-01-19 12:37:48.000000000 +0300
+++ updated/bdwgc/finalize.c	2009-02-10 21:10:30.000000000 +0300
@@ -196,6 +196,22 @@
 #     ifdef THREADS
 	LOCK();
 #     endif
+      /* Recalculate index since the table may grow.	*/
+      index = HASH2(link, log_dl_table_size);
+      /* Check again that our disappearing link not in the table. */
+      for (curr_dl = dl_head[index]; curr_dl != 0; curr_dl = dl_next(curr_dl)) {
+	if (curr_dl -> dl_hidden_link == HIDE_POINTER(link)) {
+	  curr_dl -> dl_hidden_obj = HIDE_POINTER(obj);
+#	  ifdef THREADS
+	    UNLOCK();
+#	  endif
+#	  ifndef DBG_HDRS_ALL
+	    /* Free unused new_dl returned by GC_oom_fn() */
+	    GC_free((void *)new_dl);
+#	  endif
+	  return(1);
+	}
+      }
     }
     new_dl -> dl_hidden_obj = HIDE_POINTER(obj);
     new_dl -> dl_hidden_link = HIDE_POINTER(link);
@@ -215,8 +231,8 @@
     DCL_LOCK_STATE;
     
     LOCK();
+    if (((word)link & (ALIGNMENT-1)) != 0) goto out;
     index = HASH2(link, log_dl_table_size);
-    if (((word)link & (ALIGNMENT-1))) goto out;
     prev_dl = 0; curr_dl = dl_head[index];
     while (curr_dl != 0) {
         if (curr_dl -> dl_hidden_link == HIDE_POINTER(link)) {
@@ -310,7 +326,7 @@
     ptr_t base;
     struct finalizable_object * curr_fo, * prev_fo;
     size_t index;
-    struct finalizable_object *new_fo;
+    struct finalizable_object *new_fo = 0;
     hdr *hhdr;
     DCL_LOCK_STATE;
 
@@ -328,70 +344,83 @@
     }
     /* in the THREADS case we hold allocation lock.		*/
     base = (ptr_t)obj;
-    index = HASH2(base, log_fo_table_size);
-    prev_fo = 0; curr_fo = fo_head[index];
-    while (curr_fo != 0) {
-        GC_ASSERT(GC_size(curr_fo) >= sizeof(struct finalizable_object));
-        if (curr_fo -> fo_hidden_base == HIDE_POINTER(base)) {
-            /* Interruption by a signal in the middle of this	*/
-            /* should be safe.  The client may see only *ocd	*/
-            /* updated, but we'll declare that to be his	*/
-            /* problem.						*/
-            if (ocd) *ocd = (void *) (curr_fo -> fo_client_data);
-            if (ofn) *ofn = curr_fo -> fo_fn;
-            /* Delete the structure for base. */
-                if (prev_fo == 0) {
-                  fo_head[index] = fo_next(curr_fo);
-                } else {
-                  fo_set_next(prev_fo, fo_next(curr_fo));
-                }
-            if (fn == 0) {
-                GC_fo_entries--;
-                  /* May not happen if we get a signal.  But a high	*/
-                  /* estimate will only make the table larger than	*/
-                  /* necessary.						*/
-#		if !defined(THREADS) && !defined(DBG_HDRS_ALL)
-                  GC_free((void *)curr_fo);
-#		endif
-            } else {
-                curr_fo -> fo_fn = fn;
-                curr_fo -> fo_client_data = (ptr_t)cd;
-                curr_fo -> fo_mark_proc = mp;
-		/* Reinsert it.  We deleted it first to maintain	*/
-		/* consistency in the event of a signal.		*/
-		if (prev_fo == 0) {
-                  fo_head[index] = curr_fo;
-                } else {
-                  fo_set_next(prev_fo, curr_fo);
-                }
-            }
-#	    ifdef THREADS
-                UNLOCK();
+    for (;;) {
+      index = HASH2(base, log_fo_table_size);
+      prev_fo = 0; curr_fo = fo_head[index];
+      while (curr_fo != 0) {
+	GC_ASSERT(GC_size(curr_fo) >= sizeof(struct finalizable_object));
+	if (curr_fo -> fo_hidden_base == HIDE_POINTER(base)) {
+	  /* Interruption by a signal in the middle of this	*/
+	  /* should be safe.  The client may see only *ocd	*/
+	  /* updated, but we'll declare that to be his problem.	*/
+	  if (ocd) *ocd = (void *) (curr_fo -> fo_client_data);
+	  if (ofn) *ofn = curr_fo -> fo_fn;
+	  /* Delete the structure for base. */
+	  if (prev_fo == 0) {
+	    fo_head[index] = fo_next(curr_fo);
+	  } else {
+	    fo_set_next(prev_fo, fo_next(curr_fo));
+	  }
+	  if (fn == 0) {
+	    GC_fo_entries--;
+	    /* May not happen if we get a signal.  But a high	*/
+	    /* estimate will only make the table larger than	*/
+	    /* necessary.					*/
+#	    if !defined(THREADS) && !defined(DBG_HDRS_ALL)
+	      GC_free((void *)curr_fo);
 #	    endif
-            return;
-        }
-        prev_fo = curr_fo;
-        curr_fo = fo_next(curr_fo);
-    }
-    if (ofn) *ofn = 0;
-    if (ocd) *ocd = 0;
-    if (fn == 0) {
+	  } else {
+	    curr_fo -> fo_fn = fn;
+	    curr_fo -> fo_client_data = (ptr_t)cd;
+	    curr_fo -> fo_mark_proc = mp;
+	    /* Reinsert it.  We deleted it first to maintain	*/
+	    /* consistency in the event of a signal.		*/
+	    if (prev_fo == 0) {
+	      fo_head[index] = curr_fo;
+	    } else {
+	      fo_set_next(prev_fo, curr_fo);
+	    }
+	  }
+#	  ifdef THREADS
+	    UNLOCK();
+#	  endif
+#	  ifndef DBG_HDRS_ALL
+	    if (EXPECT(new_fo != 0, FALSE)) {
+	      /* Free unused new_fo returned by GC_oom_fn() */
+	      GC_free((void *)new_fo);
+	    }
+#	  endif
+	  return;
+	}
+	prev_fo = curr_fo;
+	curr_fo = fo_next(curr_fo);
+      }
+      if (EXPECT(new_fo != 0, FALSE)) {
+	/* new_fo is returned GC_oom_fn(), so fn != 0 and hhdr != 0.	*/
+        break;
+      }
+      if (fn == 0) {
+	if (ocd) *ocd = 0;
+	if (ofn) *ofn = 0;
 #	ifdef THREADS
-            UNLOCK();
+	  UNLOCK();
 #	endif
-        return;
-    }
-    GET_HDR(base, hhdr);
-    if (0 == hhdr) {
-      /* We won't collect it, hence finalizer wouldn't be run. */
-#     ifdef THREADS
-          UNLOCK();
-#     endif
-      return;
-    }
-    new_fo = (struct finalizable_object *)
+	return;
+      }
+      GET_HDR(base, hhdr);
+      if (EXPECT(0 == hhdr, FALSE)) {
+	/* We won't collect it, hence finalizer wouldn't be run. */
+	if (ocd) *ocd = 0;
+	if (ofn) *ofn = 0;
+#	ifdef THREADS
+	  UNLOCK();
+#	endif
+	return;
+      }
+      new_fo = (struct finalizable_object *)
     	GC_INTERNAL_MALLOC(sizeof(struct finalizable_object),NORMAL);
-    if (EXPECT(0 == new_fo, FALSE)) {
+      if (EXPECT(new_fo != 0, TRUE))
+	break;
 #     ifdef THREADS
 	UNLOCK();
 #     endif
@@ -399,14 +428,19 @@
 	      GC_oom_fn(sizeof(struct finalizable_object));
       if (0 == new_fo) {
 	GC_finalization_failures++;
+	/* No enough memory.  *ocd and *ofn remains unchanged.	*/
 	return;
       }
       /* It's not likely we'll make it here, but ... */
 #     ifdef THREADS
 	LOCK();
 #     endif
+      /* Recalculate index since the table may grow and		*/
+      /* check again that our finalizer is not in the table.	*/
     }
     GC_ASSERT(GC_size(new_fo) >= sizeof(struct finalizable_object));
+    if (ocd) *ocd = 0;
+    if (ofn) *ofn = 0;
     new_fo -> fo_hidden_base = (word)HIDE_POINTER(base);
     new_fo -> fo_fn = fn;
     new_fo -> fo_client_data = (ptr_t)cd;
@@ -490,7 +524,8 @@
 #endif
 
 /* Called with held lock (but the world is running).			*/
-/* Cause disappearing links to disappear, and invoke finalizers.	*/
+/* Cause disappearing links to disappear and unreachable objects to be	*/
+/* enqueued for finalization.						*/
 void GC_finalize(void)
 {
     struct disappearing_link * curr_dl, * prev_dl, * next_dl;
diff -ru bdwgc/include/gc.h updated/bdwgc/include/gc.h
--- bdwgc/include/gc.h	2009-02-06 20:55:06.000000000 +0300
+++ updated/bdwgc/include/gc.h	2009-02-10 20:52:12.000000000 +0300
@@ -787,33 +787,38 @@
 	/* can be used to implement weak pointers easily and	*/
 	/* safely. Typically link will point to a location	*/
 	/* holding a disguised pointer to obj.  (A pointer 	*/
-	/* inside an "atomic" object is effectively  		*/
-	/* disguised.)   In this way soft			*/
-	/* pointers are broken before any object		*/
-	/* reachable from them are finalized.  Each link	*/
-	/* May be registered only once, i.e. with one obj	*/
-	/* value.  This was added after a long email discussion */
-	/* with John Ellis.					*/
-	/* Obj must be a pointer to the first word of an object */
-	/* we allocated.  It is unsafe to explicitly deallocate */
-	/* the object containing link.  Explicitly deallocating */
-	/* obj may or may not cause link to eventually be	*/
-	/* cleared.						*/
-	/* This can be used to implement certain types of	*/
-	/* weak pointers.  Note however that this generally	*/
+	/* inside an "atomic" object is effectively disguised.)	*/
+	/* In this way, weak pointers are broken before any	*/
+	/* object reachable from them gets finalized.		*/
+	/* Each link may be registered only with one obj value,	*/
+	/* i.e. all objects but the last one (link registered	*/
+	/* with) are ignored.  This was added after a long	*/
+	/* email discussion with John Ellis.			*/
+	/* link must be non-NULL (and be properly aligned).	*/
+	/* obj must be a pointer to the first word of an object */
+	/* allocated by GC_malloc or friends.  It is unsafe to	*/
+	/* explicitly deallocate the object containing link.	*/
+	/* Explicit deallocation of obj may or may not cause	*/
+	/* link to eventually be cleared.			*/
+	/* This function can be used to implement certain types	*/
+	/* of weak pointers.  Note, however, this generally	*/
 	/* requires that the allocation lock is held (see	*/
-	/* GC_call_with_allock_lock() below) when the disguised	*/
+	/* GC_call_with_alloc_lock() below) when the disguised	*/
 	/* pointer is accessed.  Otherwise a strong pointer	*/
 	/* could be recreated between the time the collector    */
 	/* decides to reclaim the object and the link is	*/
 	/* cleared.						*/
+	/* Returns 0 if registration succeeded (a new link is	*/
+	/* registered),	1 if link was already registered (with	*/
+	/* some object), 2 if registration failed for lack of	*/
+	/* memory (and GC_oom_fn did not handle the problem).	*/
 
 GC_API int GC_CALL GC_unregister_disappearing_link (void * * link);
-	/* Returns 0 if link was not actually registered.	*/
 	/* Undoes a registration by either of the above two	*/
-	/* routines.						*/
+	/* routines.  Returns 0 if link was not actually	*/
+	/* registered (otherwise returns 1).			*/
 
-/* Returns !=0  if GC_invoke_finalizers has something to do. 		*/
+/* Returns !=0 if GC_invoke_finalizers has something to do. 	*/
 GC_API int GC_CALL GC_should_invoke_finalizers(void);
 
 GC_API int GC_CALL GC_invoke_finalizers(void);


More information about the Gc mailing list