Menu
▾
▴
#2356 Lilypond segfaults
Originally created by: *anonymous
Originally created by: PhilEHol...@googlemail.com
There have been 2 reports of lilypond segfaulting on complex scores. Jay Anderson's report is here: http://lists.gnu.org/archive/html/bug-lilypond/2011-12/msg01167.html and Hu Haipeng's is here: http://old.nabble.com/problem-still-exists-in-the-latest-2.15-version-td33382315.html
Haipeng's complex score crashes LilyPond on my Vista 64-bit machine after 18 seconds compilation time, using about 350Megs memory, which is nowhere near the machine limit.
Neil suggested adding
\context {
\GrandStaff
\remove "Span_bar_stub_engraver"
}
to the score's layout block, but that does not fix the crash.
Haipeng says this does not crash on a previous version of Lilypond, but I can't check this because of new syntax in the version provided.
Originally posted by: n.putt...@gmail.com
Works for me on both scores.
I too was a bit concerned that axis_groups_ hangs around. It's obviously required between timesteps (I tried clearing it in stop_translation_timestep (), but that breaks things), but should at the very least be cleared at the end of translation in finalize ().
Originally posted by: PhilEHol...@googlemail.com
Ditto - 64 bit Ubuntu. Haipeng's crashed on .31; finishes (with errors in the log) on patched .32 version.
Originally posted by: dak@gnu.org
Since axis_groups_ is used for mapping grobs to contexts (and not the other way round), it would appear to make sense to implement it as a weak-key Scheme hash table. That way, it would at least clean itself up by and by as the key grobs are getting garbage-collected.
I am very unhappy with keeping pointers to possibly dead grobs in a C++ map that is in active use. And they keep alive contexts as values in the map that may never be looked at again.
Originally posted by: mts...@gmail.com
Go for it with the weak-key Scheme hash table - the garbage collection stuff is a bit over my head, but if it works, that's great! Otherwise, you can stay w/ the derived mark solution.
The map (in whatever form it takes) should be cleared at every stop_translation_time_step. You can add that to your patch. It's continually growing size is an oversight on my part from when I wrote the engraver.
Originally posted by: n.putt...@gmail.com
You can't clear it at every timestep. That was the first fix I tried a few weeks ago. It breaks span-bar-allow-span-bar.ly.
Originally posted by: mts...@gmail.com
I take it back then...I generally assume that when something looks wrong, I screwed it up, but there is a slight chance that I made it that way on purpose. I need to sleep more.
Originally posted by: n.putt...@gmail.com
Hehe, it's slightly worrying you suggest that, seeing as you coded it. I don't have the faintest idea what the engraver actually does. :)
Originally posted by: dak@gnu.org
Ok Mike, I was trying to change this. It starts out with
Grob *vertical_alignment = Grob::get_root_vertical_alignment ((*axis_groups_.begin ()).first);
if (!vertical_alignment) // we are at the beginning of a score, so no need for stubs
return;
axis_groups_ is a _map_. You take an _arbitrary_ element of the map (whatever happens to have the first hash bucket), then call get_root_vertical_alignment on it and work on the elements. And you never clear out the map.
This is garbage. Total garbage. Please get a night of sleep and then write down what this engraver is supposed to be doing. Not how, _what_. It is part of a very large commit. If we can't figure out what this is for and how it is related to the rest, the whole commit will need to get reverted and rewritten, and there is likely quite a bit depending on it.
Originally posted by: mts...@gmail.com
Sorry for the suggestion about axis_group_ clearing. Now that I've read it closely, I completely understand what I wrote.
The reason I don't clear out the map is cuz it's actually quite small: an axis_group_ corresponds to a staff, lyrics, dynamics, etc.. It needs to be there 100% of the time, so it's correct to not be cleared out until finalize (it should be cleared out in finalize, however).
What this engraver does is associates axis groups to their contexts, gets notified when a span bar is created, and creates SpanBarStub in all contexts that a span bar traverses (skipping over holes if allow-span-bar is set to false). Contexts need to be associated to vertical-axis-groups so that vertical sorting can happen (otherwise we don't know which contexts are higher or lower than other ones).
It seems that all that needs to be done is using some sorta SCM hash instead of the map, using derived_mark on this hash.
With respect to get_root_vertical_alignment, it can be called on an arbitrary element because the root vertical alignment of all of them will be the same save during the first timestep, where nothing has a root vertical alignment yet.
I hope this answers the "what" and the "how" (the latter of which you didn't ask for but I figured it wasimportant to state). To sum up:
"you never clear out the map."
the map should not be cleared out until a finalize method.
"You take an _arbitrary_ element of the map (whatever happens to have the first hash bucket), then call get_root_vertical_alignment on it and work on the elements."
This is because the root vertical alignment can come from any of these elements.
Originally posted by: mts...@gmail.com
Uses derived_mark to avoid segfault
http://codereview.appspot.com/5729051
Labels: Patch-new
Originally posted by: mts...@gmail.com
Uses derived_mark to avoid segfault
http://codereview.appspot.com/5729051
Originally posted by: dak@gnu.org
Regarding comment #34: now that you have written that up, put it in a comment at the top of the file. If you don't feel like weeding it out to make it self-contained, put
"from https://code.google.com/p/lilypond/issues/detail?id=2356#c34" at its top, then quote everything. As long as the web site stays around, one can then read the context of the discussion and has not just the comment itself, but also the problems that occured. But still quote the content: the usability of the source file should not depend on availability of this web site.
Related
Issues:
#2356Originally posted by: mts...@gmail.com
Uses derived_mark to avoid segfault
http://codereview.appspot.com/5729051
Originally posted by: mts...@gmail.com
Uses derived_mark to avoid segfault
http://codereview.appspot.com/5729051
Originally posted by: dak@gnu.org
Patchy the autobot says: LGTM.
Labels: Patch-review
Originally posted by: ColinPKC...@gmail.com
(No comment was entered for this change.)
Labels: -Patch-review Patch-countdown
Originally posted by: ColinPKC...@gmail.com
Counted down to 20120306, please push, although David and Neil have suggestions on Rietveld which could lead to a followup patch set. Calling for the push on this as it's marked Critical, and the suggestions sound as though they would improve the design of the patch but not the function.
Labels: -Patch-countdown Patch-push
Originally posted by: mts...@gmail.com
Pushed as [r2ccfd5a9f83819be67877522aeae7212fde3fefd].
Labels: Fixed_2_15_32
Status: Fixed
Originally posted by: PhilEHol...@googlemail.com
Verified code is in git master and that the current git master lilypond does not segfault on Haipeng's score.
Status: Verified
.