Leaking bitmap data in ree.c?

Leaking bitmap data in ree.c?

[Jeff Law]

Is it just me, or does find_removable_extensions leak bitmap data for 
INIT, KILL, GEN and TMP?

It calls bitmap_initialize on all of them, but never clears the bitmaps...

Am I missing something?

jeff

Re: Leaking bitmap data in ree.c?

[Trevor Saunders]

On Fri, Mar 18, 2016 at 01:18:16PM -0600, Jeff Law wrote:
> 
> Is it just me, or does find_removable_extensions leak bitmap data for INIT,
> KILL, GEN and TMP?
> 
> It calls bitmap_initialize on all of them, but never clears the bitmaps...
> 
> Am I missing something?

See this lovely comment for bitmap_initialize_stat () which is macroed
to bitmap_initialize ()

/* Initialize a bitmap header.  OBSTACK indicates the bitmap obstack
   to allocate from, NULL for GC'd bitmap.  */

So I think the elements for those bitmaps are just getting allocated on
the gc heap, and everything is correct though of course it would be nice
to stop using the gc there.

Trev



> 
> jeff

Re: Leaking bitmap data in ree.c?

[Jeff Law]

On 03/20/2016 09:24 PM, Trevor Saunders wrote:
> On Fri, Mar 18, 2016 at 01:18:16PM -0600, Jeff Law wrote:
>>
>> Is it just me, or does find_removable_extensions leak bitmap data for INIT,
>> KILL, GEN and TMP?
>>
>> It calls bitmap_initialize on all of them, but never clears the bitmaps...
>>
>> Am I missing something?
>
> See this lovely comment for bitmap_initialize_stat () which is macroed
> to bitmap_initialize ()
>
> /* Initialize a bitmap header.  OBSTACK indicates the bitmap obstack
>     to allocate from, NULL for GC'd bitmap.  */
>
> So I think the elements for those bitmaps are just getting allocated on
> the gc heap, and everything is correct though of course it would be nice
> to stop using the gc there.
Yea, but it's kindof silly to force the ggc system to handle this.  At 
the least a bitmap_clear would return the bitmap elements to the bitmap 
element cache (which should also be more efficient for the ggc system)

I'll resist the urge for now to apply RAII principles in this code, but 
that'd probably a much cleaner way to think about the problem in general.

jeff

Re: Leaking bitmap data in ree.c?

[Trevor Saunders]

On Mon, Mar 21, 2016 at 10:42:18AM -0600, Jeff Law wrote:
> On 03/20/2016 09:24 PM, Trevor Saunders wrote:
> >On Fri, Mar 18, 2016 at 01:18:16PM -0600, Jeff Law wrote:
> >>
> >>Is it just me, or does find_removable_extensions leak bitmap data for INIT,
> >>KILL, GEN and TMP?
> >>
> >>It calls bitmap_initialize on all of them, but never clears the bitmaps...
> >>
> >>Am I missing something?
> >
> >See this lovely comment for bitmap_initialize_stat () which is macroed
> >to bitmap_initialize ()
> >
> >/* Initialize a bitmap header.  OBSTACK indicates the bitmap obstack
> >    to allocate from, NULL for GC'd bitmap.  */
> >
> >So I think the elements for those bitmaps are just getting allocated on
> >the gc heap, and everything is correct though of course it would be nice
> >to stop using the gc there.
> Yea, but it's kindof silly to force the ggc system to handle this.  At the
> least a bitmap_clear would return the bitmap elements to the bitmap element
> cache (which should also be more efficient for the ggc system)

yeah

> I'll resist the urge for now to apply RAII principles in this code, but
> that'd probably a much cleaner way to think about the problem in general.

I worked on a couple attempts to c++ify bitmaps a while back, but never
finished any of them, but I could probably publish what I did in case
someone wants to pick that up for gcc 7.

Trev

> 
> jeff

Re: Leaking bitmap data in ree.c?

[Jeff Law]

On 03/21/2016 11:15 AM, Trevor Saunders wrote:
>
>> I'll resist the urge for now to apply RAII principles in this code, but
>> that'd probably a much cleaner way to think about the problem in general.
>
> I worked on a couple attempts to c++ify bitmaps a while back, but never
> finished any of them, but I could probably publish what I did in case
> someone wants to pick that up for gcc 7.
Can't hurt.

FWIW, bitmaps are low level and independent of trees, rtl, etc that we 
ought to (in theory) be able to class-ify them and build a real unit 
testing framework for them.

jeff

Re: Leaking bitmap data in ree.c?

[David Malcolm]

On Mon, 2016-03-21 at 11:13 -0600, Jeff Law wrote:
> On 03/21/2016 11:15 AM, Trevor Saunders wrote:
> > 
> > > I'll resist the urge for now to apply RAII principles in this
> > > code, but
> > > that'd probably a much cleaner way to think about the problem in
> > > general.
> > 
> > I worked on a couple attempts to c++ify bitmaps a while back, but
> > never
> > finished any of them, but I could probably publish what I did in
> > case
> > someone wants to pick that up for gcc 7.
> Can't hurt.
> 
> FWIW, bitmaps are low level and independent of trees, rtl, etc that
> we 
> ought to (in theory) be able to class-ify them and build a real unit 
> testing framework for them.

Something like:
https://gcc.gnu.org/ml/gcc-patches/2015-11/msg02371.html

?

Re: Leaking bitmap data in ree.c?

[Jeff Law]

On 03/21/2016 11:16 AM, David Malcolm wrote:
> On Mon, 2016-03-21 at 11:13 -0600, Jeff Law wrote:
>> On 03/21/2016 11:15 AM, Trevor Saunders wrote:
>>>
>>>> I'll resist the urge for now to apply RAII principles in this
>>>> code, but
>>>> that'd probably a much cleaner way to think about the problem in
>>>> general.
>>>
>>> I worked on a couple attempts to c++ify bitmaps a while back, but
>>> never
>>> finished any of them, but I could probably publish what I did in
>>> case
>>> someone wants to pick that up for gcc 7.
>> Can't hurt.
>>
>> FWIW, bitmaps are low level and independent of trees, rtl, etc that
>> we
>> ought to (in theory) be able to class-ify them and build a real unit
>> testing framework for them.
>
> Something like:
> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg02371.html
Like that, or complete instantiation outside GCC.

jeff

Re: Leaking bitmap data in ree.c?

[Richard Biener]

On Mon, Mar 21, 2016 at 6:19 PM, Jeff Law <law@redhat.com> wrote:
> On 03/21/2016 11:16 AM, David Malcolm wrote:
>>
>> On Mon, 2016-03-21 at 11:13 -0600, Jeff Law wrote:
>>>
>>> On 03/21/2016 11:15 AM, Trevor Saunders wrote:
>>>>
>>>>
>>>>> I'll resist the urge for now to apply RAII principles in this
>>>>> code, but
>>>>> that'd probably a much cleaner way to think about the problem in
>>>>> general.
>>>>
>>>>
>>>> I worked on a couple attempts to c++ify bitmaps a while back, but
>>>> never
>>>> finished any of them, but I could probably publish what I did in
>>>> case
>>>> someone wants to pick that up for gcc 7.
>>>
>>> Can't hurt.
>>>
>>> FWIW, bitmaps are low level and independent of trees, rtl, etc that
>>> we
>>> ought to (in theory) be able to class-ify them and build a real unit
>>> testing framework for them.
>>
>>
>> Something like:
>> https://gcc.gnu.org/ml/gcc-patches/2015-11/msg02371.html
>
> Like that, or complete instantiation outside GCC.

I don't like more C++-ification just for the sake of it.  Like the alloc-pool
C++ification caused more issues than it fixed.

I'd happily approve sth like an auto_bitmap class though (bah, we can't
have auto <X> as auto is already taken!).

Notice that using bitmap_initialize is a micro-optimization to avoid one
indirection.  I doubt it matters for REE.

Also keep in mind bitmap statistics - leaving bitmaps to GC only like
REE seems to do will mess them up.  Bitmaps that have used
bitmap_initialize and allocating from GC need to call bitmap_clear
on them.  In fact REE should simply use &bitmap_default_obstack,
I think that it uses GC is an oversight...

Richard.

> jeff

Re: Leaking bitmap data in ree.c?

[Trevor Saunders]

On Mon, Mar 21, 2016 at 11:13:07AM -0600, Jeff Law wrote:
> On 03/21/2016 11:15 AM, Trevor Saunders wrote:
> >
> >>I'll resist the urge for now to apply RAII principles in this code, but
> >>that'd probably a much cleaner way to think about the problem in general.
> >
> >I worked on a couple attempts to c++ify bitmaps a while back, but never
> >finished any of them, but I could probably publish what I did in case
> >someone wants to pick that up for gcc 7.
> Can't hurt.

ok, finally got around to that.  The git mirror now has branches
tbsaunde/bitmap tbsaunde/bitmap2 and tbsaunde/bitvec.  I think the first
two actually aren't very valuable, trying add a ctor / dtor to
bitmap_head itself seems like a poor idea bound to run into some strange
use of bitmaps.  It seems a lot safer to add a auto_bitmap and move
things over to it.  On the other hand the bitvec branch is trying to
build something sbitmap like, but trying to use vec to manage the
storage instead of doing it manually.  I'm not sure why I didn't finish that
idea.

Trev

> 
> FWIW, bitmaps are low level and independent of trees, rtl, etc that we ought
> to (in theory) be able to class-ify them and build a real unit testing
> framework for them.
> 
> jeff

Re: Leaking bitmap data in ree.c?

[Trevor Saunders]

On Sat, Apr 02, 2016 at 08:49:57PM -0400, Trevor Saunders wrote:
> On Mon, Mar 21, 2016 at 11:13:07AM -0600, Jeff Law wrote:
> > On 03/21/2016 11:15 AM, Trevor Saunders wrote:
> > >
> > >>I'll resist the urge for now to apply RAII principles in this code, but
> > >>that'd probably a much cleaner way to think about the problem in general.
> > >
> > >I worked on a couple attempts to c++ify bitmaps a while back, but never
> > >finished any of them, but I could probably publish what I did in case
> > >someone wants to pick that up for gcc 7.
> > Can't hurt.
> 
> ok, finally got around to that.  The git mirror now has branches
> tbsaunde/bitmap tbsaunde/bitmap2 and tbsaunde/bitvec.  I think the first
> two actually aren't very valuable, trying add a ctor / dtor to
> bitmap_head itself seems like a poor idea bound to run into some strange
> use of bitmaps.  It seems a lot safer to add a auto_bitmap and move
> things over to it.  On the other hand the bitvec branch is trying to
> build something sbitmap like, but trying to use vec to manage the
> storage instead of doing it manually.  I'm not sure why I didn't finish that
> idea.

oh, and now I do remember why, it looks like it depends on making vec
call destructors when objects in the vector are removed which makes
sense, but it makes implementing pop () tricky.

Trev

> 
> Trev
> 
> > 
> > FWIW, bitmaps are low level and independent of trees, rtl, etc that we ought
> > to (in theory) be able to class-ify them and build a real unit testing
> > framework for them.
> > 
> > jeff