Re: 2.19.81 aborts on many .ly files when compiled with gcc8 -Wp, -D_GLIBCXX_ASSERTIONS

[Mamoru TASAKA]

Hello:

David Kastrup wrote on 05/09/2018 08:23 PM:
> Mamoru TASAKA <address@hidden> writes:
>
> > And the attached proposal patch should fix this issue. Please review this.
>
> > >From 463aabe95b2dd3856c928c5e6917eda2138f3aa4 Mon Sep 17 00:00:00 2001
> > From: Mamoru TASAKA <address@hidden>
> > Date: Tue, 8 May 2018 22:34:41 +0900
> > Subject: [PATCH] Fix out-of-bounds access detected by -D_GLIBCXX_ASSERTIONS
> >
> > file: lily/beam.cc
> > In Beam::calc_beam_segments(), when on_beam_bound is true, access for segs[] is 
> > currenctly out-of-bounds.
> > So calculate on_beam_bound first to avoid that invalid access. In this case, 
> > neighbor_seg is not used
> > anyway, so this is okay.
> > ---
> >   lily/beam.cc | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/lily/beam.cc b/lily/beam.cc
> > index dff62168af..d32624a4ff 100644
> > --- a/lily/beam.cc
> > +++ b/lily/beam.cc
> > @@ -457,15 +457,15 @@ Beam::calc_beam_segments (SCM smob)
> >             Beam_stem_segment const &seg = segs[j];
> >             for (LEFT_and_RIGHT (event_dir))
> >               {
> > -              Beam_stem_segment const &neighbor_seg = segs[j + event_dir];
> > +              bool on_beam_bound = (event_dir == LEFT) ? j == 0
> > +                                   : j == segs.size () - 1;
> > +              Beam_stem_segment const &neighbor_seg = segs[on_beam_bound ? 0 : 
> > j + event_dir];
> >                 // TODO: make names clearer? --jneem
> >                 // on_line_bound: whether the current segment is on the 
> > boundary of the WHOLE beam
> >                 // on_beam_bound: whether the current segment is on the 
> > boundary of just that part
> >                 //   of the beam with the current beam_rank
> >                 bool on_line_bound = (seg.dir_ == LEFT) ? seg.stem_index_ == 0
> >                                      : seg.stem_index_ == stems.size () - 1;
> > -              bool on_beam_bound = (event_dir == LEFT) ? j == 0
> > -                                   : j == segs.size () - 1;
> >                 bool inside_stem = (event_dir == LEFT)
> >                                    ? seg.stem_index_ > 0
> >                                    : seg.stem_index_ + 1 < stems.size ();
>
> Well spotted but this patch is butt-ugly by calculating a nonsense
> reference in the troublesome case that is then being ignored.
>
> Since neighbor_seg is not even getting used more than twice and that in
> the same expression, I'd just write out segs[j + event_dir] instead and
> let the optimizer do the rest.  The problematic expression is then not
> even generated because of short-circuit evaluation.

So would you mean below?
diff --git a/lily/beam.cc b/lily/beam.cc
index dff62168af..b318f0834d 100644
--- a/lily/beam.cc
+++ b/lily/beam.cc
@@ -457,7 +457,6 @@ Beam::calc_beam_segments (SCM smob)
           Beam_stem_segment const &seg = segs[j];
           for (LEFT_and_RIGHT (event_dir))
             {
-              Beam_stem_segment const &neighbor_seg = segs[j + event_dir];
               // TODO: make names clearer? --jneem
               // on_line_bound: whether the current segment is on the boundary 
of the WHOLE beam
               // on_beam_bound: whether the current segment is on the boundary 
of just that part
@@ -471,9 +470,9 @@ Beam::calc_beam_segments (SCM smob)
                                  : seg.stem_index_ + 1 < stems.size ();
 
               bool event = on_beam_bound
-                           || abs (seg.rank_ - neighbor_seg.rank_) > 1
+                           || abs (seg.rank_ - segs[j + event_dir].rank_) > 1
                            || (abs (vertical_count) >= seg.max_connect_
-                               || abs (vertical_count) >= 
neighbor_seg.max_connect_);
+                               || abs (vertical_count) >= segs[j + 
event_dir].max_connect_);
 
               if (!event)
                 // Then this edge of the current segment is irrelevant because 
it will

I am okay with either of these.

Regards,
Mamoru