Fix memory corruption in operands

[Jan Hubicka]

> On Mon, 2007-01-22 at 19:21 +0100, Jan Hubicka wrote:
> > > On Mon, 2007-01-22 at 17:36 +0100, Jan Hubicka wrote:
> 
> > > 
> > > you will of course lose that if you go to a global list because the
> > > cache will never be freed up until the last function is processed.  It
> > > boils down to how well we manage the annotations. As far as I know, they
> > > should be handled reasonably well.  If we don't, They certainly should
> > > be anyway :-)
> > 
> > Well, the only place we call free_ssa_operands is from delete_ssa, so
> > all stmt annotations for statments elliminated by DCE and others are
> > left dead for a moment I blieve.
> 
> Hmm. There is code in here that is no longer working the way I
> originally intended. I shall have to make a few changes :-)
> free_ssa_operands() isnt serving a lot of purpose where it is. its
> simply clearing out the annotation before ssa is destroyed, which
> doesn't do much when it comes to the cache  since it gets completely
> destroyed anyway.  The pointers to the operands are irrelevant.

Yep :)
> 
> It looks like we dont actually have a way of catching removed stmts
> earlier. Part for the reason for that is because bsi_remove() is
> responsible for removing a stmt from from the stmt list, not actually
> destroying the stmt.  A removed stmt could immediately be linked back
> into the stmt list, and then you would of course still want the
> operands.

Yes, we have weakly defined what bsi_remove should do.  At the moment it
just removes the stmt from list and few tables, while it keeps
annotations/SSA names and others.
Zdenek hit the problem with his (unfinished) work on removing unused SSA
names from the list
http://www.mail-archive.com/gcc@gcc.gnu.org/msg22722.html
I think he is right making bsi_remove destructive (ie remove/free as
much from statement as we manage explicitely expecting it to not be used
again).
Perhaps the not too obvious BOOL argument can be eliminated by forking
bsi_remove into bsi_remove (doing all the removal) and bsi_unlink
(expecting the statement to be linked back again).
> 
> Do the stmt annotations for these deleted stmts ever get freed either? I
> cant seem to find a routine that does that, so they are just garbage
> collected when the stmt is collected?  

Yes, they are garbage collected.  Sadly we reference to dead statements
in many places, so I think it would be profitable to actually clear the
link to annotation so it actually can be ggc_collected.
See the other thread on ggc_unreferenced_object - I would like to
progress on eliminating those dangling pointers to statements, but it is
harder that one would think.
> 
> Perhaps we need in addition to bsi_remove() a bsi_destroy() which also
bsi_remove/bsi_destroy or bsi_remove/bsi_unlink :) But yes, we are on
the same track.
> frees up anything associated with it.  It could then call
> free_ssa_operands() which could then be fixed to us the free lists.
> 
> I'll take a look at that. I thought we had actually done the
> bsi_destroy() before, but perhaps it was just the remove_eh flag which
> has been added.. is there ever a case where we ask to remove the eh
> info, and we AREN'T destroying the stmt?  Anyhoo, I'll take a look.

Well, we can make the statement untrapping and then remove EH, but we
probably won't do that via remove_stmt.
> 
> > Just to get some hard data, at the end of compilation we have for
> > combine.c:
> > 
> > tree-ssanames.c:143 (make_ssa_name)                 1367136: 4.1%          0: 0.0%          0: 0.0%          0: 0.0%      28482
> > tree-inline.c:2754 (copy_tree_r)                    1372400: 4.2%          0: 0.0%          0: 0.0%      17356: 0.5%      37765
> > tree-dfa.c:189 (create_stmt_ann)                    1433432: 4.3%     866376: 6.3%          0: 0.0%          0: 0.0%      41068
> > genrtl.c:17 (gen_rtx_fmt_ee)                        1885404: 5.7%          0: 0.0%       1956: 0.1%          0: 0.0%     157280
> > Total                                              32996633         13710922          3145510          3307173          1365803
> > source location                                     Garbage            Freed             Leak         Overhead            Times
> > 
> > (this show that stmt annotations are one of major memory consumers, but
> > also that amount of them allocated by left unfreeded to them freeded is
> > something like 4:6.  This seems like pretty big leak for freelists.  It
> > is of course bigger when amount of dead code increase, ie for example in
> > tramp3d).
> 
> We dont actually have a free list for stmt annotations right?  they are
> just garbaged collected.  If we destroyed a stmt as mentioned above by
> also clearing out the annotation field, would that help at all? probably
> not. they would get collected at the same time as the stmt that was
> delinked by bsi_remove()  anyway.

We call gcc_free on stmt annotations just before releasing going to RTL.
I used this logic as base for the numbers above (freed/leaking), since
we clear SSA operands shortly before on the very same set of statements.
Otherwise they are just left to be collected.
Exlicit allocation on thsese (via freelists or ggc_free) would make
sense. During early optimization on C++ we tend to produce tons of
statements to just remove them shortly.
> 
> what exactly does that "leak" number mean?  The operand cache is all
> pointed to via a linked list of large segments which operands are
> sub-allocated from and all freed at once as segments, so it should never
> show up as a "leak". And annotations are all GC'd, so I wouldnt think
> they would ever show up as a leak.  Or am I misinterpreting what a leak
> is?

Garbage is memory that has been garbage collected out, Freed is memory
explicitely freed via ggc_free, Leak is memory still referenced.
They are not leaking in the numbers above.
The other set of numbers was run just after IPA, so at this time all the
annotations/operands for all the live functions was included.
> 
> how do I get this memory report?

You need to compile with --enable-gather-detailed-mem-stats and then
you can use -fmem-report to get numbers after whole compilation,
-fpre-ipa-mem-report -fpost-ipa-mem-report to get allocations before and
after.
> 
> > Which is one of few places where we get the freelists right, sadly.
> 
> well, it will certainly help if I add a bsi_destroy() and add the caches
> to the free list, and make the free list global to the compilation unit.
> I'll have a look and see if I can help.

Thanks a lot!

Honza
> 
> Andrew