| Message ID | 20141128.193713.462002670.kkojima@rr.iij4u.or.jp |
|---|---|
| State | New |
| Headers | show |
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);
Richard Biener <richard.guenther@gmail.com> wrote:
> Huh? Wasn't this fixed by Mike a few weeks ago?
Ugh. I've missed that thread. Sorry for the noise.
Regards,
kaz
On Nov 28, 2014, at 4:03 AM, Richard Biener <richard.guenther@gmail.com> wrote: > 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? While I had testing on my target, there were plenty of other regressions from trunk intermixed, so I wanted to do a normal bootstrap on Linux which I had not done yet. Kaz, you can check your patch, if the same, pretty sure it is, it has been approved and you can just check your version in, since you have the testing on it. Richard didn't want the extra testing my patch. >> * 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);