Re: the new gc asserts in master

[Han-Wen Nienhuys <hanwen@xs4all.nl>]

Ludovic Courtès escreveu:
> Han-Wen Nienhuys <hanwen@xs4all.nl> writes:
> 
>> I am pushing a fix for this to master.
> 
> Will you care to post and discuss your patches before pushing them?

Over the last weeks I have seen little discussion on your patches, eg.

  commit 582a4997abc8b34ac6caf374fda8ea3ac65bd571
  Author: Ludovic Courtès <ludo@gnu.org>
  Date:   Mon Aug 25 11:20:02 2008 +0200

      Use $(GCC_CFLAGS) for `-Werror' et al. so that it's not used to compile
      Gnulib code.

  commit c95514b3b41c8e335ada863f8abb99cc4af9abe1
  Author: Ludovic Courtès <ludo@gnu.org>
  Date:   Thu Aug 14 00:15:03 2008 +0200

      Remove the now useless `qthreads.m4'.

Were pushed without review.  There was a post on

  commit 450be18dfffd496ef14e1c921953e6f179727ab4
  Author: Ludovic Courtès <ludo@gnu.org>
  Date:   Thu Jul 17 00:17:56 2008 +0200

      Handle lack of `struct dirent64' and `readdir64_r ()' on HP-UX 11.11.

but it was after the fact

  Hi,

  FYI, I committed the attached patch, which handles the lack of `struct
  dirent64' and `readdir64_r ()' on HP-UX 11.11 (and possibly other
  versions).

I'm assuming here that you -in good community spirit- don't consider
yourself to be above your own rules.

In other words: you can't do this.  If you want people to discuss
before pushing you should set the good example.

> This is all the more important that the patches don't seem to have any
> relation with the problem at hand:
> 
> f85ea2a85fcdd051f432964806f044c0301d0945 Merge branch 'master' of git://git.sv.gnu.org/guile into nits
> 487b9dec2ea6b88ddbc6fbd17f445ddb197aebc5 Only sanity check numbers if SCM_DEBUG_CELL_ACCESSES is unset.
> 80237dcc7783b4d94ecf1d987deb9306d61735a0 Set SRCPROP{PLIST,COPY} through a macro, so SCM_DEBUG_CELL_ACCESSES compiles.

This in reference to GUILE not compiling with SCM_DEBUG_CELL_ACCESSES 

The commit message says,

    Set SRCPROP{PLIST,COPY} through a macro, so SCM_DEBUG_CELL_ACCESSES compiles.

how much clearer do you want this message to be?  It is fixing a
compilation problem for a certain preprocessor define.  It doesn't
pretend to fix x86-64. You were complaining before that my changes
were too large and should have been split up.  This patch is split
up.

Can you make up your mind?

> Do you think you can come up with a fix within the next few days?

In the spirit of your undocumented development and
community standards, I am including below a patch for this
problem. Let's discuss this complex change first to decide whether it
is worthy for inclusion in the oh-so-active GUILE repository.

> Otherwise, I'm inclined to revert the offending commits in `master' and
> wait for a signal from you (i.e., a patch or merge request posted to the
> mailing list, *not* a commit on `master').  It would make it easier for
> us to play with `master' in the meantime.

> Besides, avoid pushing from an non-up-to-date repo: this yields to
> automatic merges like the one above, which is annoying as it makes
> history harder to follow.  Better pull first, then merge your changes,
> then push.

Can you document your requirements upfront instead complaining after the fact?

[lilydev@haring guile]$ grep -i merge HACKING 
[lilydev@haring guile]$ 

[lilydev@haring guile]$ grep -i pull HACKING 
[lilydev@haring guile]$ 

>>> even the lazy smob case I wrote about here:
>>>
>>>     http://thread.gmane.org/gmane.lisp.guile.user/6372
>> I would classify the use of mark bits outside of the mark phase as outside
>> of the defined API.  If you want to have weak pointer semantics, use
>> a weak hashtable, or implement reference counting on the C side.
> 
> That's a reasonable argument, but it's something we should not change
> without discussing it first.  For instance, it may be important to study
> why Guile-GNOME had to resort to this, and how it could avoid it,
> instead of just gratuitously breaking it.

I'm not suggesting to change without discussing; this message rather
is the start of the discussion.  I think reference counting is the
correct solution for this, as far as I understand the problem from the
quoted message.

The use of scm_gc_mark() outside of GC is fundamentally broken, since it
creates race conditions in the presence of threads.

-- 
 Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen


From ccd010e15ec0ddf285b75911739e85866d2d865c Mon Sep 17 00:00:00 2001
From: Han-Wen Nienhuys <hanwen@lilypond.org>
Date: Wed, 27 Aug 2008 10:48:06 -0300
Subject: [PATCH] Kludge around x86-64 GC runtime checks.

2008-08-27  Han-Wen Nienhuys  <hanwen@lilypond.org>

	* gc.c (scm_i_gc): Don't sanity check numbers on x64, while we
	investigate a real fix.

---
 libguile/gc.c      |    4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/libguile/gc.c b/libguile/gc.c
index 8e8769c..a0b3080 100644
--- a/libguile/gc.c
+++ b/libguile/gc.c
@@ -597,9 +597,9 @@ scm_i_gc (const char *what)
   scm_i_sweep_all_segments ("GC", &scm_i_gc_sweep_stats);
   scm_check_deprecated_memory_return ();
 
+#if (SCM_DEBUG_CELL_ACCESSES == 0 && SCM_SIZEOF_UNSIGNED_LONG == 4)
   /* Sanity check our numbers. */
-
-#if (SCM_DEBUG_CELL_ACCESSES == 0)
+  /* TODO(hanwen): figure out why the stats are off on x64_64. */
   /* If this was not true, someone touched mark bits outside of the
      mark phase. */
   assert (scm_cells_allocated == scm_i_marked_count ());
-- 
1.5.5.1