bug#19883: Correction for backtrace

[Ludovic Courtès]

David Kastrup <address@hidden> skribis:

> Is there a reason you are not using the test program provided with this
> bug report?

Sorry, I had overlooked it.  Please assume good faith.

With this patch:

diff --git a/smobs.tcc b/smobs.tcc
index c8b9dfe..b1d61b7 100644
--- a/smobs.tcc
+++ b/smobs.tcc
@@ -132,9 +132,6 @@ void Smob_base<Super>::init ()
   // types in order to be able to compare them.  The other comparisons
   // are for static member functions and thus are ordinary function
   // pointers which work without those contortions.
-  if (static_cast<SCM (Super::*)()>(&Super::mark_smob) !=
-      static_cast<SCM (Super::*)()>(&Smob_base<Super>::mark_smob))
-    scm_set_smob_mark (smob_tag_, Super::mark_trampoline);
   scm_set_smob_print (smob_tag_, Super::print_trampoline);
   if (&Super::equal_p != &Smob_base<Super>::equal_p)
     scm_set_smob_equalp (smob_tag_, Super::equal_p);

“./test 1000 1000 10000” works fine.

However, it’s not representative since the C++ objects in this test do
not hold ‘SCM’ references, AFAICS.

> LilyPond's GUILEv2 branch is currently out of order again since 2.0.11
> changed encoding mechanisms _again_

Please submit a different bug report.

>>> A pointer to a C++ structure does not appear to protect the
>>> corresponding SMOB data and free_smob calls the delete operator which
>>> calls destructors and clobbers the memory area.
>>
>> Oh, I was mistaken in my previous message.  GC scans the stack and the
>> GC-managed heap (stuff allocated with GC_MALLOC/scm_gc_malloc et al.),
>> but it does *not* scan the malloc/new heap.
>>
>> So indeed, C++ objects that hold references to ‘SCM’ objects, such as
>> instances of ‘Smob<X>’, must either have a mark function, or they must
>> be allocated with scm_gc_malloc.
>>
>> Would it be possible to add a ‘new’ operator to ‘Smob’ that uses
>> ‘scm_gc_malloc’, and a ‘delete’ operator that uses ‘scm_gc_free’?
>
> It would not help since many of the references are stored in STL
> containers (like std::vector <Grob *>) which have their data
> allocated/deallocated separately from the memory area of the structure
> itself.

Oh, OK.  Still, I don’t think this is a problem because each C++ object
has a corresponding SMOB, and said SMOB is GC-protected; thus the C++
object itself is also GC-protected until the SMOB is unprotected.

Here’s the patch I’ve ended up with:

diff --git a/smobs.hh b/smobs.hh
index 3701280..a41a645 100644
--- a/smobs.hh
+++ b/smobs.hh
@@ -263,6 +263,20 @@ private:
 protected:
   Smob () : self_scm_ (SCM_UNDEFINED), protection_cons_ (SCM_EOL) { };
 public:
+  static void *operator new (std::size_t size)
+  {
+    /* This C++ object is referred to by the corresponding SMOB, which is
+       itself GC-protected.  Thus, no need to protect the C++ object.  */
+    return scm_gc_malloc (size, "lily-smob");
+  }
+
+  static void operator delete (void *thing)
+  {
+    /* Nothing to do: the GC will reclaim memory for THING when it deems
+       appropriate.  */
+    // printf ("delete %p\n", thing);
+  }
+
   static size_t free_smob (SCM obj)
   {
     delete Smob_base<Super>::unregister_ptr (obj);
diff --git a/smobs.tcc b/smobs.tcc
index c8b9dfe..0eb0c30 100644
--- a/smobs.tcc
+++ b/smobs.tcc
@@ -31,7 +31,7 @@ template <class Super>
 SCM
 Smob_base<Super>::mark_trampoline (SCM arg)
 {
-  return (Super::unsmob (arg))->mark_smob ();
+  abort ();
 }
 
 template <class Super>
@@ -60,14 +60,14 @@ template <class Super>
 SCM
 Smob_base<Super>::mark_smob ()
 {
-  return SCM_UNSPECIFIED;
+  abort ();
 }
 
 template <class Super>
 size_t
 Smob_base<Super>::free_smob (SCM)
 {
-  return 0;
+  abort ();
 }
 
 template <class Super>
@@ -125,16 +125,11 @@ void Smob_base<Super>::init ()
   // While that's not a consideration for type_p_name_, it's easier
   // doing it like the rest.
 
-  if (&Super::free_smob != &Smob_base<Super>::free_smob)
-    scm_set_smob_free (smob_tag_, Super::free_smob);
   // Old GCC versions get their type lattice for pointers-to-members
   // tangled up to a degree where we need to typecast _both_ covariant
   // types in order to be able to compare them.  The other comparisons
   // are for static member functions and thus are ordinary function
   // pointers which work without those contortions.
-  if (static_cast<SCM (Super::*)()>(&Super::mark_smob) !=
-      static_cast<SCM (Super::*)()>(&Smob_base<Super>::mark_smob))
-    scm_set_smob_mark (smob_tag_, Super::mark_trampoline);
   scm_set_smob_print (smob_tag_, Super::print_trampoline);
   if (&Super::equal_p != &Smob_base<Super>::equal_p)
     scm_set_smob_equalp (smob_tag_, Super::equal_p);
diff --git a/test.cc b/test.cc
index b5ab476..6bbd435 100644
--- a/test.cc
+++ b/test.cc
@@ -41,9 +41,7 @@ Family::Family (int totals, int branch)
 SCM
 Family::mark_smob ()
 {
-  for (int i = 0; i < kids.size (); i++)
-    scm_gc_mark (kids[i]->self_scm ());
-  return SCM_EOL;
+  abort ();
 }
 
 int
@@ -51,7 +49,11 @@ Family::count ()
 {
   int sum = 1;
   for (int i = 0; i < kids.size (); i++)
-    sum += kids[i]->count ();
+    {
+      sum += kids[i]->count ();
+      if ((i % 100) == 0)
+       scm_gc ();
+    }
   return sum;
 }
 

It works on this test case, and, assuming I correctly understand
LilyPond’s use case, it may be a viable solution for LilyPond.  Could
you give it a try and report back?

> Frankly, I don't get the current strategy of GUILE: basically any use of
> scm_set_smob_mark will result in a function that can be called with
> garbage from a smob that has already been deallocated via the function
> registered with scm_set_smob_free.

That’s not true.  For example, the GnuTLS bindings use the exact same
code for 1.8 and 2.0 without any problems; it’s surely a simpler example
than LilyPond, of course, but still.

~~~~~~~

In the interest of the Guile and LilyPond projects, I would like to
improve our communication.

Let me explain what my personal expectations are.  First, like many
others, I’m a volunteer with limited bandwidth.  Thus, I really prefer
bug reports that are as concise as possible and to-the-point; I can’t
afford to read so much.

Second, I don’t want to mix issues: this bug report is about a specific
GC issue, not about an encoding issue.  It’s better for me (and everyone
I guess) when a bug report remains focused.

Third, we must fix these LilyPond vs. Guile 2 issues.  You and others
have spent a lot of time on this already.  I think it would help to get
everyone involved on both sides.  Thus, could you Cc: this bug report to
the LilyPond developer list, or the corresponding LilyPond bug report?
This is really important to me.

Please don’t take it personally or anything.  I’m really trying hard to
see how we can improve the collaboration between the two projects.

Thanks,
Ludo’.