Re: GCC plugins & GGC & explicit gcc_free

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

On August 29, 2014 5:29:43 PM CEST, Basile Starynkevitch <basile@starynkevitch.net> wrote:
>Hello All,
>
>[[I know that this is a sensitive issue, and I also know that I am in
>the minority believing that a garbage collection is useful inside the
>GCC compiler]]
>
>How should plugins deal with data that is explicitly gcc_free-d outside
>of ggc_collect?
>
>In particular, plugins are apparently allowed to have their own GTY-ed
>data (which is very good, I certainly don't want that feature to
>disappear). I'm particularly thinking of "static analysis" (or
>"observing") plugins, which would take advantage of the GCC compiler to
>add some extra measures and analysis but which won't change anything
>observable in the GCC internal representations (e.g. plugins that won't
>add or remove any Gimple inside existing functions, for instance).
>
>I believe that such "observing" plugins should certainly be allowed to
>register a pass which would put some data (e.g. tree-s or
>basic_block-s)
>inside some GTY-ed variable and examine it much later, perhaps even at
>PLUGIN_FINISH time (and certainly at PLUGIN_FINISH_UNIT time). This
>would make sense if such a plugin would like to make some detailed
>statistics, serialize *some* of the Gimple or Tree-s in a database,
>etc.
>
>However, such a plugin could crash, because at several occasions
>ggc_free is *forcibly* called *outside* of the garbage collector - and
>there is no mechanism (finalization, plugin hooks) to communicate that
>deletion to the plugin. So such a plugin will keep a *stale* data
>referenced by its GTY-ed roots.
>
>I have a concrete example within my MELT branch -currently close to GCC
>4.9-  & plugin (see http://gcc-melt.org/ if you never heard of MELT).
>the justcountipa pass I have in file gcc/melt/xtramelt-ana-simple.melt
>near lines 840 and following. It stores some global MELT data singleton
>of CLASS_JUSTCOUNTIPA_DATA in the MELT pass, and that data stays
>available, so is never freed by the GGC. Unfortunately, that data
>contains (very indirectly) some tree-s and edge-s which have been
>*explicitly* ggc_free-d, e.g. by execute_todo calling indirectly
>cleanup_tree_cfg calling indirectly free_edge.
>
>If you are interested here is the offending backtrace (with a
>watchpoint
>triggered when ggc_free is overwriting, by enable-checking code, some
>edge still kept in MELT global data thru a GTY-ed variable) 
>
>0  in memset of ../sysdeps/x86_64/memset.S:65
>1  in ggc_free of /usr/src/Lang/basile-melt-gcc/gcc/ggc-page.c:1544
>2  in free_edge of /usr/src/Lang/basile-melt-gcc/gcc/cfg.c:92
>3  in remove_edge_raw of /usr/src/Lang/basile-melt-gcc/gcc/cfg.c:351
>4  in remove_edge of /usr/src/Lang/basile-melt-gcc/gcc/cfghooks.c:418
>5  in remove_phi_nodes_and_edges_for_unreachable_block
>of /usr/src/Lang/basile-melt-gcc/gcc/tree-cfg.c:1934
>6  in remove_bb of /usr/src/Lang/basile-melt-gcc/gcc/tree-cfg.c:2016
>7  in delete_basic_block
>of /usr/src/Lang/basile-melt-gcc/gcc/cfghooks.c:565
>8  in remove_forwarder_block
>of /usr/src/Lang/basile-melt-gcc/gcc/tree-cfgcleanup.c:529
>9  in cleanup_tree_cfg_bb
>of /usr/src/Lang/basile-melt-gcc/gcc/tree-cfgcleanup.c:633
>10 in cleanup_tree_cfg_1
>of /usr/src/Lang/basile-melt-gcc/gcc/tree-cfgcleanup.c:675
>11 in cleanup_tree_cfg_noloop
>of /usr/src/Lang/basile-melt-gcc/gcc/tree-cfgcleanup.c:731
>12 in cleanup_tree_cfg
>of /usr/src/Lang/basile-melt-gcc/gcc/tree-cfgcleanup.c:786
>13 in execute_function_todo
>of /usr/src/Lang/basile-melt-gcc/gcc/passes.c:1811
>14 in do_per_function of
>/usr/src/Lang/basile-melt-gcc/gcc/passes.c:1574
>15 in execute_todo of /usr/src/Lang/basile-melt-gcc/gcc/passes.c:1887
>16 in execute_one_pass
>of /usr/src/Lang/basile-melt-gcc/gcc/passes.c:2247
>17 in execute_pass_list
>of /usr/src/Lang/basile-melt-gcc/gcc/passes.c:2286
>18 in execute_pass_list
>of /usr/src/Lang/basile-melt-gcc/gcc/passes.c:2287
>19 in do_per_function_toporder
>of /usr/src/Lang/basile-melt-gcc/gcc/passes.c:1630
>20 in execute_ipa_pass_list
>of /usr/src/Lang/basile-melt-gcc/gcc/passes.c:2617
>21 in ipa_passes of /usr/src/Lang/basile-melt-gcc/gcc/cgraphunit.c:2043
>22 in compile of /usr/src/Lang/basile-melt-gcc/gcc/cgraphunit.c:2174
>23 in finalize_compilation_unit
>of /usr/src/Lang/basile-melt-gcc/gcc/cgraphunit.c:2329
>
>If I continue the above execution my MELT plugin crashes (in GGC
>marking
>routines) because the stale edge has been free-d and reused later.
> 
>Of course, a dirty workaround is for me to clear manually that data
>before it could be seen by the GGC marking garbage collector. And such
>a
>workaround is working. But I really find it shameful.
>
>Unfortunately, the data which are manually freed outside of ggc_collect
>(so which should perhaps not be kept without care in GTY-ed variables
>inside plugins) is not exactly documented.
>
>From my biased point of view, I would much prefer if ggc_free was
>called
>*only* from ggc_collect. Sadly, I believe that few GCC people would
>agree with that. Or we could have ggc_free called only from
>ggc_collect,
>and add a new ggc_becomes_freeable routine (and every call to ggc_free
>outside of ggc_collect would be replaced by a call to
>ggc_becomes_freeable which for example would call ggc_free if not
>plugin
>is loaded, and stays a no-op if some plugins are loaded...).
>
>Perhaps we could improve the situation e.g.
>
>
>   document more when and which data is cleaned up
>
>  add a finalizer plugin hook for the few static calls to ggc_free, but
>that probably would be quite expensive (e.g. we'll run that hook from
>free_edge in gcc/cfg.c ...) because ggc_free is probably called a big
>lot of times.
>
>   more realistically, add plugin hooks in cleanup routines, notably in
>cleanup_cfg file gcc/cfgcleanup.c
>
>
>Please comment on this. If we agree on something I'll try to propose
>some patch to GCC 5.0 during its stage 1. 

You are in the same situation as any other pass would be.  Don't hang on things that can get stale.

There is no point in keeping a pointer to a deleted edge.

Of course we should make things more explicit here and move all data structures out of GC that are explicitly freed.  Work in that direction is welcome.  The CFG is in GC memory because it indirectly refers to trees (the single most reason why things are in GC space, fixable nowadays with custom GC marker routines)

Richard.

>
>Regards.