Re: [RFC] Wrong "current" pointers with bitmap_ior and bitmap_ior_and_compl

[Richard Biener <richard dot guenther at gmail dot com>]

On Fri, Nov 28, 2014 at 11:37 AM, Kaz Kojima <kkojima@rr.iij4u.or.jp> wrote:
> Hi,
>
> I've got odd ICEs with some bitmap operations when working on
> sh-lra branch.
> bitmap_ior and bitmap_ior_and_compl are the bitmap operations
> for DST = A | B and DST = A | (B & ~KILL) respectively.  It
> looks they could make bitmaps with the wrong "current" pointer.
> One small example is
>
> DST: [(bit 0, bit 127), indx=0] <-> [(bit 151), indx=1] -> 0
> A:   [(bit 141), indx=1] -> 0
> B:   [(bit 142), indx=1] -> 0
>
> where assuming that BITMAP_ELEMENT_ALL_BITS is 128.
> In this case, bitmap_ior makes DST into the list
>
> [(bit 141, bit 142), indx=1] <-> [(bit 151), indx=1] -> 0
>
> and unlinks the second element with bitmap_elt_clear_from.
> If the "current" pointer of DST pointed the 2nd element,
> the current pointer points the element freed from the list:
>
> DST: [(bit 141, bit 142), indx=1] -> 0
> DST->current: [(bit 151), indx=1]
>
> even after bitmap_elt_clear_from done.
> bitmap_elt_clear_from has the code to fix the current pointer
> for the correct list of which elements have strictly ascending
> indx values.  It doesn't work for the above list because its
> elements have the same indx.
> It seems that bitmap_and, bitmap_and_compl and bitmap_xor have
> the line to avoid this issue already.  The patch below adds
> the same line to bitmap_ior and bitmap_ior_and_compl.
> The patch is tested on i686-pc-linux-gnu with bootstrap and
> the top level "make -k check".

Huh?  Wasn't this fixed by Mike a few weeks ago?

Richard.

> Regards,
>         kaz
> --
>         * bitmap.c (bitmap_ior): Reset the current pointer before calling
>         bitmap_elt_clear_from.
>         (bitmap_ior_and_compl): Likewise.
>
> diff --git a/bitmap.c b/bitmap.c
> index 8f7f306..ace6aea 100644
> --- a/bitmap.c
> +++ b/bitmap.c
> @@ -1595,6 +1595,8 @@ bitmap_ior (bitmap dst, const_bitmap a, const_bitmap b)
>    if (dst_elt)
>      {
>        changed = true;
> +      /* Ensure that dst->current is valid.  */
> +      dst->current = dst->first;
>        bitmap_elt_clear_from (dst, dst_elt);
>      }
>    gcc_checking_assert (!dst->current == !dst->first);
> @@ -1951,6 +1953,8 @@ bitmap_ior_and_compl (bitmap dst, const_bitmap a, const_bitmap b, const_bitmap k
>    if (dst_elt)
>      {
>        changed = true;
> +      /* Ensure that dst->current is valid.  */
> +      dst->current = dst->first;
>        bitmap_elt_clear_from (dst, dst_elt);
>      }
>    gcc_checking_assert (!dst->current == !dst->first);