Re: The relationship between SCM and scm_t_bits.

[Dirk Herrmann]

Marius Vollmer wrote:

>  Dirk Herrmann <address@hidden> writes:
>
> > Certainly, the way we convert between scm_t_bits and SCM is
> > implementation dependent. However, the definitions for scm_t_bits
> > and SCM are IMO a very good way to provide an abstraction of some
> > of this uncleanlyness. And, with today's definitions of scm_t_bits
> > and SCM, the heap _must_ hold scm_t_bits variables. Please
> > explain, why you think that it is cleaner to say it only holds
> > scheme objects if in fact it does not.
>
>  The reason is that there exits code that does essentially this:
>
>  scm_t_bits heap_field;
>
>  SCM value = whatever (); SCM *ptr = (SCM *)&heap_field; *ptr = value;

I assume that you mean that heap_field is actually an element of the heap.

We already had the discussion that I suggest to discourage this style of 
coding since it violates a potential write barrier and will lead to 
problems if we ever switch to a generational garbage collection. Despite 
of this discussion, you nevertheless seem to have decided that you do 
not want to discourage this style of coding. That's OK, it's perfectly 
fair to make that decision if you are aware of the consequences. I would 
just prefer if such decisions were explicitly stated, in order to avoid 
confusion for both guile developers and users. Please consider that, as 
long as such a decision is not official, people who have ever followed 
discussions about the implications of such coding on generational gc 
might put some effort into avoiding such code. We can well spare them 
and ourselves this effort then.

> > What is the reason to change a paradigm, which has for several
> > years worked quite nicely, is easily understood, and has helped to
> > find and probably also to avoid a bunch of errors?
>
>  I don't think that the paradigm has changed fundamentally. It has
>  been strengthened, if you will. The distinction between scm_t_bits
>  and SCM is still there.
>
>  We don't just cast between SCM and scm_t_bits, we use SCM_PACK and
>  SCM_UNPACK. Except sometimes a scm_t_bits variable is stored into
>  via a SCM* pointer, totally ruining the care PACk/UNPACK abstraction.
>  That exception has now been removed. I see that as an unconditional
>  improvement, don't you?

As said above, I accept if you decide to allow such a coding style. 
Then, you are right, people should have the option to have SCM pointers 
into the heap. I just doubt that the current solution is elegant. On the 
contrary: I think that it introduces an uncleanlyness for exactly the 
other type of scenario, namely if someone needs to have a scm_t_bits 
pointer into the heap. In particular, I have a problem with the 
following lines of code.

 In gc.h:

   #define SCM_GC_CELL_WORD(x, n)   (SCM_UNPACK (SCM_GC_CELL_OBJECT 
((x), (n))))

   This expression has a SCM value as an intermediate result, which is 
definitely unclean, since the SCM value might (in contrast to the 
definition of SCM) not represent a valid scheme object.

 In numbers.h:

   #define SCM_I_BIG_MPZ(x) (*((mpz_t *) (SCM_CELL_OBJECT_LOC((x),1))))

   This expression has a SCM* as an intermediate result, although in 
this case we _know_ that we are actually pointing to a scm_t_bits value.

My suggestion is just, to remove this uncleanlyness by going one step 
further, namely to make scm_t_cell a union of scm_t_bits and SCM values. 
IMO, this is the most elegant approach anyway, since it reflects the 
actual situation best. I enclose a patch to accomplish this. Please 
consider the patch and, if you like, comment on it. I had it presented 
before, but if I remember correctly, there has not been a definite 
decision on it. Thus, I would just go ahead and apply it within the next 
couple of days.

Best regards,
Dirk

Index: guile-core/NEWS
===================================================================
RCS file: /cvsroot/guile/guile/guile-core/NEWS,v
retrieving revision 1.438
diff -u -r1.438 NEWS
--- guile-core/NEWS     19 Aug 2004 17:54:37 -0000      1.438
+++ guile-core/NEWS     20 Aug 2004 17:44:57 -0000
@@ -744,16 +744,6 @@
     SCM_SYMBOL_HASH           -> scm_hashq
     SCM_SYMBOL_INTERNED_P     -> scm_symbol_interned_p
 
-** SCM_CELL_WORD_LOC has been deprecated.
-
-Use the new macro SCM_CELL_OBJECT_LOC instead, which return a pointer
-to a SCM, as opposed to a pointer to a scm_t_bits.
-
-This was done to allow the correct use of pointers into the Scheme
-heap.  Previously, the heap words were of type scm_t_bits and local
-variables and function arguments were of type SCM, making it
-non-standards-conformant to have a pointer that can point to both.
-
 ** New macros SCM_SMOB_DATA_2, SCM_SMOB_DATA_3, etc.
 
 These macros should be used instead of SCM_CELL_WORD_2/3 to access the
Index: guile-core/libguile/ChangeLog
===================================================================
RCS file: /cvsroot/guile/guile/guile-core/libguile/ChangeLog,v
retrieving revision 1.2128
diff -u -r1.2128 ChangeLog
--- guile-core/libguile/ChangeLog       20 Aug 2004 13:33:39 -0000      1.2128
+++ guile-core/libguile/ChangeLog       20 Aug 2004 17:45:05 -0000
@@ -1,3 +1,21 @@
+2004-05-24  Dirk Herrmann  <address@hidden>
+
+       * deprecated.h (SCM_CELL_WORD_LOC): Un-deprecated.
+
+       * gc.h (scm_t_cell): Redefined to hold a union, since each cell
+       element either holds a scm_t_bits value or a SCM value.
+
+       (SCM_GC_CARD_BVEC, SCM_GC_SET_CARD_BVEC, SCM_GC_GET_CARD_FLAGS,
+       SCM_GC_SET_CARD_FLAGS, SCM_GC_CELL_OBJECT, SCM_GC_CELL_WORD,
+       SCM_GC_SET_CELL_OBJECT, SCM_GC_SET_CELL_WORD): Modified to work
+       with the new scm_t_cell.
+
+       (SCM_CELL_WORD_LOC): Un-deprecated.
+
+       * numbers.h (SCM_I_BIG_MPZ): Use SCM_CELL_WORD_LOC instead of
+       SCM_CELL_OBJECT_LOC, since we are not dealing with scheme objects
+       here.
+
 2004-08-20  Marius Vollmer  <address@hidden>
 
        * eval.c (scm_lookupcar1): Report "Variable used before given a
Index: guile-core/libguile/deprecated.h
===================================================================
RCS file: /cvsroot/guile/guile/guile-core/libguile/deprecated.h,v
retrieving revision 1.29
diff -u -r1.29 deprecated.h
--- guile-core/libguile/deprecated.h    19 Aug 2004 16:49:42 -0000      1.29
+++ guile-core/libguile/deprecated.h    20 Aug 2004 17:45:05 -0000
@@ -291,11 +291,6 @@
 
 #define SCM_VALIDATE_OPDIR(pos, port) SCM_MAKE_VALIDATE (pos, port, OPDIRP)
 
-/* Deprecated because we can not safely cast a SCM* to a scm_t_bits*
- */
-
-#define SCM_CELL_WORD_LOC(x, n)   ((scm_t_bits*)SCM_CELL_OBJECT_LOC((x),(n)))
-
 /* Users shouldn't know about INUMs.
  */
 
Index: guile-core/libguile/gc.h
===================================================================
RCS file: /cvsroot/guile/guile/guile-core/libguile/gc.h,v
retrieving revision 1.118
diff -u -r1.118 gc.h
--- guile-core/libguile/gc.h    19 Aug 2004 16:48:37 -0000      1.118
+++ guile-core/libguile/gc.h    20 Aug 2004 17:45:05 -0000
@@ -36,8 +36,10 @@
 
 typedef struct scm_t_cell
 {
-  SCM word_0;
-  SCM word_1;
+  union {
+    scm_t_bits word;
+    SCM object;
+  } elements[2];
 } scm_t_cell;
 
 /*
@@ -70,15 +72,17 @@
 
 
 #define SCM_GC_CARD_N_HEADER_CELLS 1
-#define SCM_GC_CARD_N_CELLS        256
-#define SCM_GC_SIZEOF_CARD             SCM_GC_CARD_N_CELLS * sizeof 
(scm_t_cell)
+#define SCM_GC_CARD_N_CELLS 256
+#define SCM_GC_SIZEOF_CARD SCM_GC_CARD_N_CELLS * sizeof (scm_t_cell)
 
-#define SCM_GC_CARD_BVEC(card)  ((scm_t_c_bvec_long *) ((card)->word_0))
+#define SCM_GC_CARD_BVEC(card) \
+  ((scm_t_c_bvec_long *) ((card)->elements[0].word))
 #define SCM_GC_SET_CARD_BVEC(card, bvec) \
-    ((card)->word_0 = (SCM) (bvec))
-#define SCM_GC_GET_CARD_FLAGS(card) ((long) ((card)->word_1))
+  ((card)->elements[0].word = (scm_t_bits) (bvec))
+#define SCM_GC_GET_CARD_FLAGS(card) \
+  ((long) ((card)->elements[1].word))
 #define SCM_GC_SET_CARD_FLAGS(card, flags) \
-    ((card)->word_1 = (SCM) (flags))
+  ((card)->elements[1].word = (flags))
 
 #define SCM_GC_GET_CARD_FLAG(card, shift) \
  (SCM_GC_GET_CARD_FLAGS (card) & (1L << (shift)))
@@ -141,12 +145,13 @@
  * in debug mode.  In particular these macros will even work for free cells,
  * which should never be encountered by user code.  */
 
-#define SCM_GC_CELL_OBJECT(x, n) (((SCM *)SCM2PTR (x)) [n])
-#define SCM_GC_CELL_WORD(x, n)   (SCM_UNPACK (SCM_GC_CELL_OBJECT ((x), (n))))
+#define SCM_GC_CELL_WORD(x, n) (((SCM2PTR (x))->elements[n]).word)
+#define SCM_GC_CELL_OBJECT(x, n) (((SCM2PTR (x))->elements[n]).object)
 
-#define SCM_GC_SET_CELL_OBJECT(x, n, v) ((((SCM *)SCM2PTR (x)) [n]) = (v))
+#define SCM_GC_SET_CELL_OBJECT(x, n, v) \
+  (((SCM2PTR (x))->elements[n]).object = (v))
 #define SCM_GC_SET_CELL_WORD(x, n, v)  \
-  (SCM_GC_SET_CELL_OBJECT ((x), (n), SCM_PACK (v)))
+  (((SCM2PTR (x))->elements[n]).word = (v))
 
 #define SCM_GC_CELL_TYPE(x) (SCM_GC_CELL_OBJECT ((x), 0))
 
@@ -214,7 +219,10 @@
   (SCM_GC_SET_CELL_OBJECT ((x), 1, (v)))
 
 
-#define SCM_CELL_OBJECT_LOC(x, n) (SCM_VALIDATE_CELL((x), &SCM_GC_CELL_OBJECT 
((x), (n))))
+#define SCM_CELL_WORD_LOC(x, n) \
+  (SCM_VALIDATE_CELL((x), &SCM_GC_CELL_WORD ((x), (n))))
+#define SCM_CELL_OBJECT_LOC(x, n) \
+  (SCM_VALIDATE_CELL((x), &SCM_CELL_OBJECT ((x), (n))))
 #define SCM_CARLOC(x)             (SCM_CELL_OBJECT_LOC ((x), 0))
 #define SCM_CDRLOC(x)             (SCM_CELL_OBJECT_LOC ((x), 1))
 
Index: guile-core/libguile/numbers.h
===================================================================
RCS file: /cvsroot/guile/guile/guile-core/libguile/numbers.h,v
retrieving revision 1.93
diff -u -r1.93 numbers.h
--- guile-core/libguile/numbers.h       9 Aug 2004 23:32:14 -0000       1.93
+++ guile-core/libguile/numbers.h       20 Aug 2004 17:45:05 -0000
@@ -142,7 +142,7 @@
 #define SCM_COMPLEX_IMAG(x) (SCM_COMPLEX_MEM (x)->imag)
 
 /* Each bignum is just an mpz_t stored in a double cell starting at word 1. */
-#define SCM_I_BIG_MPZ(x) (*((mpz_t *) (SCM_CELL_OBJECT_LOC((x),1))))
+#define SCM_I_BIG_MPZ(x) (*((mpz_t *) (SCM_CELL_WORD_LOC ((x), 1))))
 #define SCM_BIGP(x) (!SCM_IMP (x) && SCM_TYP16 (x) == scm_tc16_big)
 
 #define SCM_NUMBERP(x) (SCM_I_INUMP(x) || SCM_NUMP(x))