On Fri, Aug 29, 2014 at 07:58:14PM +0200, Richard Biener wrote: > On August 29, 2014 5:29:43 PM CEST, Basile Starynkevitch > <bas...@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)
I thought it was also because of pch? I'm hoping we'll soon be at a point where everything uses the overload based gc scheme currently used for user gc which should hopefully make custom marking easier, but pch will still be a big pain, and in fact is by far the worst part of implementing marking functions. Trev > > Richard. > > > > >Regards. > >
