Issue 2376: Automatic footnotes on \null markups causes unexpected results

Issue 2376: Automatic footnotes on \null markups causes unexpected results

The page layout routines passed around pointers to stencils rather
indiscriminately and worked on them rather than on stencil copies.

Sort of copy-on-write without the copy.  This tries working on actual
copies where appropriate.

[MikeSol, 2012-03-07 07:30:42]

http://codereview.appspot.com/5755058/diff/1/lily/page-breaking.cc
File lily/page-breaking.cc (right):

http://codereview.appspot.com/5755058/diff/1/lily/page-breaking.cc#newcode584
lily/page-breaking.cc:584: }
Just a C++ question - do these lines implicitly call the copy constructor to
create footnote_stencil so that footnote_stencil is a copy of foot?  I'm not
familiar with this syntax for copying, so I just wanted to be sure that this is
what these lines were doing.

[dak, 2012-03-07 07:40:58]

http://codereview.appspot.com/5755058/diff/1/lily/page-breaking.cc
File lily/page-breaking.cc (right):

http://codereview.appspot.com/5755058/diff/1/lily/page-breaking.cc#newcode584
lily/page-breaking.cc:584: }
On 2012/03/07 07:30:42, MikeSol wrote:
> Just a C++ question - do these lines implicitly call the copy constructor to
> create footnote_stencil so that footnote_stencil is a copy of foot?  I'm not
> familiar with this syntax for copying, so I just wanted to be sure that this
is
> what these lines were doing.

The first line calls the assignment operator.  The second line just resets the
foot pointer to point at footnote_stencil.

It would likely be cleaner to just pass a pointer to footnote_stencil (which is
constructed as a null Stencil as far as I can see) in either case.  Since the
special case "foot == 0" would then be ruled out in advance (do you need the
error message?  Can add_footnotes_to_footer deal with a null stencil?) one could
in a more C++-like manner pass foot in by reference instead of by pointer.

I did not want to meddle with the logic of the code: I just made sure that it
copied what was needed.  If you say that add_footnotes_to_footer can deal with
getting a null stencil for starters, that's what I would prefer using. 
Incidentally, the length of the axes of a null stencil should turn out to be 0,
so this would remove a few special cases as well.

[dak, 2012-03-07 08:48:56]

http://codereview.appspot.com/5755058/diff/1/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (right):

http://codereview.appspot.com/5755058/diff/1/lily/page-layout-problem.cc#newc...
lily/page-layout-problem.cc:343: return footnote_separator;
Uh, Mike?

You call a Scheme function for calculating a stencil, then unsmob the stencil
and return a pointer to the unsmobbbed entity.  The SCM expression is not kept
around in any manner.  How on Earth is the Scheme garbage collector supposed to
guess that the Stencil for which you have retained a pointer but no referring
SCM is not supposed to be collected right away?

That's not just buggy code, it is also a function call interface that can't be
made to work.  You can't return pointers to unsmobbed entities for which no SCM
reference is retained anywhere.

Given the level and amount of code you write, it might be worth investing the
time rereading the garbage collection chapter of the Guile manual.

I'll be preparing a fix for this one as well.

[mike_apollinemike.com, 2012-03-07 12:52:41]

On Mar 7, 2012, at 8:48 AM, dak@gnu.org wrote:

> Given the level and amount of code you write, it might be worth
> investing the time rereading the garbage collection chapter of the Guile
> manual.
> 

You're right that I know nothing about guile's garbage collection...it'd help to
know this.  I'll investigate.  It'd also be good to do a little write-up for the
CG w/ garbage collection dos and don'ts in the LilyPond base (i.e. what
mark_smob, derived_mark, unsmob_thingee, smobbed_copy, self_scm & co. all mean).

Cheers,
MS

[dak, 2012-03-07 13:25:54]

"mike@apollinemike.com" <mike@apollinemike.com> writes:

> On Mar 7, 2012, at 8:48 AM, dak@gnu.org wrote:
>
>> Given the level and amount of code you write, it might be worth
>> investing the time rereading the garbage collection chapter of the Guile
>> manual.
>> 
>
> You're right that I know nothing about guile's garbage
> collection...it'd help to know this.

In a nutshell: if an SCM object is not referenced, the garbage collector
is free to throw it and its contents away.

As a reference counts:

a) getting marked using scm_gc_mark during garbage collection (mark
phase) because another referenced object does this.  All Scheme data
structures do that to their members (with the exception of "weak" parts
of a hash table).  Our own "Scheme" objects need to do this to their
contents via their own mark subroutines (like derived_mark).

b) getting permanently "protected".  Many of our own data structures
when created with new start out in protected state (since they don't
have an SCM referencing them yet, hard to avoid) and convert into SCM
via a call to unprotect () once they are properly initialized so that
calling their mark routine does not cause uninitialized garbage to be
marked.

c) having an SCM type variable in the stack referencing it.  This is a
somewhat shaky operation since the compiler may optimize variables away
when they are no longer needed.  If that is a concern, you can use
scm_remember_upto_here_1 and its cousins to keep the SCM variables
alive.

Strictly speaking, code like

      Stencil *s = unsmob_stencil (Text_interface::interpret_markup (layout, pro
      if (!s)
        {
          programming_error ("Your numbering function needs to return a stencil.
          footnote_number_markups.push_back (SCM_EOL);
          footnote_number_stencils.push_back (Stencil (Box (Interval (0, 0), Int
        }
      else
        {
          footnote_number_markups.push_back (markup);
          footnote_number_stencils.push_back (*s);
        }
      counter++;

is already borderline.  From the time of unsmob_stencil, the returned
markup is no longer protected as an SCM object.  If
footnote_number_markups.push_back were to cause Scheme garbage
collection to trigger, *s would likely already be trashed.

If we get a multithreaded garbage collector at one point of time, you
first need to assign the result of interpret_markup to an SCM variable
and put an scm_remember_upto_here_1 on this variable after the last use
of the unsmobbed stencil pointer.  Of course, if the unsmobbed object is
part of a larger protected data structure, you need not worry all that
much.  Much of the Grob * in our code base is rather laissez-faire about
protection because the grobs are usually registered somewhere else.  For
example, Grob_info does not bother protecting its contents at all if I
remember correctly because it assumes that they will live longer than
the Grob_info because of unrelated reasons.

> I'll investigate.  It'd also be good to do a little write-up for the
> CG w/ garbage collection dos and don'ts in the LilyPond base
> (i.e. what mark_smob, derived_mark, unsmob_thingee, smobbed_copy,
> self_scm & co. all mean).

A smobbed_copy is a Scheme object with the same contents as the original
but not using it (and thus also not protecting it from collection).  A
self_scm is a Scheme object referencing the original.  unsmob gives a
pointer to the unwrapped object.  mark_smob is called during the mark
phase of a referenced object and should in turn mark all SCM objects
that it references.

-- 
David Kastrup