Fix crash when unknown grob name is passed to ly:engraver-make-grob

Fix crash when unknown grob name is passed to ly:engraver-make-grob

[Pavel Roskin, 2012-03-01 23:02:57]

I've seen it many times that Lilypond would crash during my experiments with
Scheme code.  This time I decided to track it down.  It's my first ever fix to
the C++ code in Lilypond, so please check my coding style.

[dak, 2012-03-01 23:35:30]

On 2012/03/01 23:02:57, Pavel Roskin wrote:
> I've seen it many times that Lilypond would crash during my experiments with
> Scheme code.  This time I decided to track it down.  It's my first ever fix to
> the C++ code in Lilypond, so please check my coding style.

How is that a fix?  You return a non-grob value that will wreak havoc later on. 
The only thing that would seem to make some sense here is throwing a (Scheme)
exception.  It will likely cause an abort of the program, but in a better
controlled manner that can be caught and debugged from Scheme.

[Pavel Roskin, 2012-03-03 01:22:30]

There are only two calls to scm_throw in Lilypond, and I could not find out
whether it should be added to engraver.cc or engraver-scheme.cc.  I added it to
the former so that other callers to Engraver::internal_make_grob could benefit. 
I could not figure out how to catch an exception.  Without catching, I'm
getting:

GNU LilyPond 2.15.32
Processing `bad.ly'
Parsing...
Interpreting music... 
fatal error: failed files: "bad.ly"

The warning "cannot create grob" is not shown, and neither is the name of the
grob that caused it.  I could put is before the exception, but this is different
from the style on the existing code.

[hanwenn, 2012-03-03 04:17:45]

On Fri, Mar 2, 2012 at 10:22 PM,  <plroskin@gmail.com> wrote:
> There are only two calls to scm_throw in Lilypond, and I could not find
> out whether it should be added to engraver.cc or engraver-scheme.cc.  I
> added it to the former so that other callers to
> Engraver::internal_make_grob could benefit.  I could not figure out how
> to catch an exception.  Without catching, I'm getting:
>
> GNU LilyPond 2.15.32
> Processing `bad.ly'
> Parsing...
> Interpreting music...
> fatal error: failed files: "bad.ly"
>
> The warning "cannot create grob" is not shown, and neither is the name
> of the grob that caused it.  I could put is before the exception, but
> this is different from the style on the existing code.

try running lilypond --verbose


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

[Pavel Roskin, 2012-03-03 04:38:36]

I'm not getting any interesting information:

...
  [/usr/local/share/lilypond/2.15.32/ly/context-mods-init.ly]
  [/usr/local/share/lilypond/2.15.32/ly/engraver-init.ly]]
 [bad.ly]
Interpreting music... 
[/usr/local/share/lilypond/2.15.32/fonts/otf/emmentaler-20.otf]
fatal error: failed files: "bad.ly"

Sure, I can move the programming_error() call before scm_throw() and print
whatever I want.  I just want to follow the coding style of Lilypond.  And the
existing style is such that scm_throw() is only used twice, both for more
serious errors in the input.

[dak, 2012-03-03 05:43:50]

On 2012/03/03 04:38:36, Pavel Roskin wrote:
> I'm not getting any interesting information:
> 
> ...
>   [/usr/local/share/lilypond/2.15.32/ly/context-mods-init.ly]
>   [/usr/local/share/lilypond/2.15.32/ly/engraver-init.ly]]
>  [bad.ly]
> Interpreting music... 
> [/usr/local/share/lilypond/2.15.32/fonts/otf/emmentaler-20.otf]
> fatal error: failed files: "bad.ly"
> 
> Sure, I can move the programming_error() call before scm_throw() and print
> whatever I want.  I just want to follow the coding style of Lilypond.  And the
> existing style is such that scm_throw() is only used twice, both for more
> serious errors in the input.

You can call ly:error.  It does not exactly raise an exception but
exit(1).  Or you call programming_error and return a dummy value of the
expected type, something that makes it possible to continue.

But continuing with a wrong-type value is only going to cause trouble later on.

[dak, 2012-03-20 23:29:55]

http://codereview.appspot.com/5715053/diff/2001/lily/engraver-scheme.cc
File lily/engraver-scheme.cc (right):

http://codereview.appspot.com/5715053/diff/2001/lily/engraver-scheme.cc#newco...
lily/engraver-scheme.cc:42: programming_error ("cannot create grob");
This gives no helpful indication of where the error occured, and triggers
problems later in unrelated code.  That's not better than a segfault: neither is
better than "something that shouldn't happened somewhere".

You could check whether "cause" is a grob in which case you can call unsmob_grob
(cause)->programming_error on it in order to give the error message a _source_
indicator, or a stream-event in which case unsmob_event (cause)->origin
()->programming_error would deliver a suitable source indicator for the error.

[Pavel Roskin, 2020-02-20 06:27:37]

Thanks for the ping, it's been a long time! Sadly, I haven't used lilypond for
years. I tried to update my patch, but I cannot spend too much time on it.
I see that my change to engraver.cc is no longer needed, as there is a check
there that was missing back at the time. The change to engraver-scheme.cc still
applies, but I see that the comments here indicate that it's incomplete and
doesn't provide much value.
I have no recollection what I was doing with ly:engraver-make-grob, so I googled
to that symbol and found an implementation of ambitus at
http://lilypond.org/doc/v2.19/Documentation/snippets/contexts-and-engravers
I replaced "Ambitus" with "Ambituus" and I see the check added in engraver.cc
being triggered ("programming error: No grob definition found for `Ambituus’")
but the segfault happens not in ly_engraver_make_grob but in
Item::spanned_rank_interval.
So I think this patch is irrelevant and can be discarded.