| Message ID | 559E63A5.1000108@mentor.com |
|---|---|
| State | New |
| Headers | show |
Hi, On Thu, 9 Jul 2015, Tom de Vries wrote: > > > Given this I think the call to gt_ggc_mx is superfluous because it > > > wouldn't work relyably for multi-step dependencies anyway. Hence a > > > situation that works with that call in place, and breaking without > > > it is actually a bug waiting to be uncovered. > > > > Attached patch tries to get multi-step dependencies right, without > > using iteration-till-fixed-point. > > And for the record, attached patch implements a naive iterative > approach. What uses do multi-step dependencies have? As in, I think this goes into the wrong direction, we lived without this since years, so why should this situation be handled at all? It's about cache hash tables, so they shouldn't contain anything that is only pointed to at by entries in those tables. If anything we rather should check, that calling gt_ggc_mx on anything retained in the hash tables doesn't generate newly live objects. Ciao, Michael.
On 09/07/15 14:24, Michael Matz wrote: > Hi, > > On Thu, 9 Jul 2015, Tom de Vries wrote: > >>>> Given this I think the call to gt_ggc_mx is superfluous because it >>>> wouldn't work relyably for multi-step dependencies anyway. Hence a >>>> situation that works with that call in place, and breaking without >>>> it is actually a bug waiting to be uncovered. >>> >>> Attached patch tries to get multi-step dependencies right, without >>> using iteration-till-fixed-point. >> >> And for the record, attached patch implements a naive iterative >> approach. > > What uses do multi-step dependencies have? As in, I think this goes into > the wrong direction, we lived without this since years, so why should this > situation be handled at all? It's about cache hash tables, so they > shouldn't contain anything that is only pointed to at by entries in those > tables. > > If anything we rather should check, that calling gt_ggc_mx on anything > retained in the hash tables doesn't generate newly live objects. > Hi Michael, I'm trying to get to a defined policy for what is allowed for caches. Either forbidding or allowing multi-step dependencies, I don't really mind. Until now, we didn't have a good way of allowing them. I came up with a runtime efficient but not exhaustive variant, which I posted here: https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00711.html. As contrast and for the record, I posted the exhaustive but not runtime efficient variant here: https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00730.html. I managed to write a patch series that implements the forbidding of multi-step dependencies. I'll post this soon. Thanks, - Tom
On 12/07/15 17:43, Tom de Vries wrote: > On 09/07/15 14:24, Michael Matz wrote: >> Hi, >> >> On Thu, 9 Jul 2015, Tom de Vries wrote: >> >>>>> Given this I think the call to gt_ggc_mx is superfluous because it >>>>> wouldn't work relyably for multi-step dependencies anyway. Hence a >>>>> situation that works with that call in place, and breaking without >>>>> it is actually a bug waiting to be uncovered. >>>> >>>> Attached patch tries to get multi-step dependencies right, without >>>> using iteration-till-fixed-point. >>> >>> And for the record, attached patch implements a naive iterative >>> approach. >> >> What uses do multi-step dependencies have? As in, I think this goes into >> the wrong direction, we lived without this since years, so why should >> this >> situation be handled at all? It's about cache hash tables, so they >> shouldn't contain anything that is only pointed to at by entries in those >> tables. >> >> If anything we rather should check, that calling gt_ggc_mx on anything >> retained in the hash tables doesn't generate newly live objects. >> > > Hi Michael, > > I'm trying to get to a defined policy for what is allowed for caches. > Either forbidding or allowing multi-step dependencies, I don't really mind. > > Until now, we didn't have a good way of allowing them. I came up with a > runtime efficient but not exhaustive variant, which I posted here: > https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00711.html. > As contrast and for the record, I posted the exhaustive but not runtime > efficient variant here: > https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00730.html. > > I managed to write a patch series that implements the forbidding of > multi-step dependencies. I'll post this soon. > https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00970.html Thanks, - Tom
Hi, On Sun, 12 Jul 2015, Tom de Vries wrote: > > I'm trying to get to a defined policy for what is allowed for caches. > > Either forbidding or allowing multi-step dependencies, I don't really > > mind. I think forbidding is the way to go, because ... > > I managed to write a patch series that implements the forbidding of > > multi-step dependencies. I'll post this soon. > > https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00970.html ... aha, it finds bugs! So you actually had to changes some hashes to be non-caching for this to work, and it's all some decl-to-debug-something maps (well, of course, otherwise you wouldn't have run into the bug you're trying to fix in the first place). I think this hints at actual bugs that needs fixing in the gomp branch: As you analyzed in PR 66714, eventually a decl A is replaced by decl B, but its debug-expr is simply copied, and that one itself refers to decls (let's call them D*) that meanwhile are removed. Now, as the D* decls are not in any other data structure (otherwise they would have been marked) the typical actions that needed to have been done for them (like e.g. associating debug info with them, allocating them to some stack or register place) i.e. anything that needed to be done for normal decls won't have been done. So the debug info generator in this case, when it sees those D* decls can't do its work, e.g. debug info generated for D* won't refer to the real place containing the value, because also the generated code itself doesn't refer to D* anymore. This also hints at other problems (which might not actually occur in the case at hand, but still): the contents of DECL_VALUE_EXPR is the "real" thing containing the value of a decl (i.e. a decl having a value-expr doesn't itself occur in the code anymore), be it a decl itself, or some expression (which might also refer to decls). Now, in PR 66714 you analyzed that one of those D* was removed from the function, which should have happened only because no code referred to anymore, i.e. D* was also rewritten to some other D'* (if it weren't rewritten and D* was referred to in code, you would have created a miscompilation). At that point also the DECL_VALUE_EXPRs need to be rewritten to refer to D'*, not to D* anymore. Implementing multi-step maps or making the hashmaps non-caching doesn't solve any of the above problems, it merely forces some DECLs in the compiler to remain live but that actually have no meaning in their context. So, I think this makes it pretty clear that those hashmaps should remain caching maps, and that multi-step deps in caches should be disallowed, and that the underlying problem should rather be fixed (and the checking code against multi-step-deps should be added to the compiler). Ciao, Michael.
On 13/07/15 15:43, Michael Matz wrote: > Hi, > > On Sun, 12 Jul 2015, Tom de Vries wrote: > >>> I'm trying to get to a defined policy for what is allowed for caches. >>> Either forbidding or allowing multi-step dependencies, I don't really >>> mind. > > I think forbidding is the way to go, because ... > >>> I managed to write a patch series that implements the forbidding of >>> multi-step dependencies. I'll post this soon. >> >> https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00970.html > > ... aha, it finds bugs! So you actually had to changes some hashes to be > non-caching for this to work, and it's all some decl-to-debug-something > maps (well, of course, otherwise you wouldn't have run into the bug you're > trying to fix in the first place). I think this hints at actual bugs that > needs fixing in the gomp branch: > > As you analyzed in PR 66714, eventually a decl A is replaced by decl B, > but its debug-expr is simply copied, and that one itself refers to decls > (let's call them D*) that meanwhile are removed. > > Now, as the D* decls are not in any other data structure (otherwise they > would have been marked) the typical actions that needed to have been done > for them (like e.g. associating debug info with them, allocating them to > some stack or register place) i.e. anything that needed to be done for > normal decls won't have been done. So the debug info generator in this > case, when it sees those D* decls can't do its work, e.g. debug info > generated for D* won't refer to the real place containing the value, > because also the generated code itself doesn't refer to D* anymore. > > This also hints at other problems (which might not actually occur in the > case at hand, but still): the contents of DECL_VALUE_EXPR is the "real" > thing containing the value of a decl (i.e. a decl having a value-expr > doesn't itself occur in the code anymore), be it a decl itself, or some > expression (which might also refer to decls). Now, in PR 66714 you > analyzed that one of those D* was removed from the function, which should > have happened only because no code referred to anymore, i.e. D* was also > rewritten to some other D'* (if it weren't rewritten and D* was referred > to in code, you would have created a miscompilation). At that point also > the DECL_VALUE_EXPRs need to be rewritten to refer to D'*, not to D* > anymore. > Thanks for looking into the PR. I suspected that these things were wrong, but I have no knowledge of this part of the compiler, so I was not sure. > Implementing multi-step maps or making the hashmaps non-caching doesn't > solve any of the above problems I'm not saying that making those hashmaps non-caching solves any of these problems. I'm saying that it decouples fixing the policy (for which I have a patch) from fixing the issues that allow us to use these 3 as caches again (for which there are no patches yet). The advantage of having a policy in place is that we won't regress for tables still marked as cache (or new tables marked as cache). So blocking committing the policy on those issues makes no sense IMHO. Thanks, - Tom >, it merely forces some DECLs in the > compiler to remain live but that actually have no meaning in their > context. > > So, I think this makes it pretty clear that those hashmaps should remain > caching maps, and that multi-step deps in caches should be disallowed, and > that the underlying problem should rather be fixed (and the checking code > against multi-step-deps should be added to the compiler). > > > Ciao, > Michael. >
Hi, On Mon, 13 Jul 2015, Tom de Vries wrote: > > Implementing multi-step maps or making the hashmaps non-caching > > doesn't solve any of the above problems > > I'm not saying that making those hashmaps non-caching solves any of > these problems. Ah, I didn't mean to imply this, I meant to imply that enforcing policy as you do is a good thing because it finds bugs, and that the policy to be enforced should be forbidding multi-step deps :) > I'm saying that it decouples fixing the policy (for which I have a > patch) from fixing the issues that allow us to use these 3 as caches > again (for which there are no patches yet). The advantage of having a > policy in place is that we won't regress for tables still marked as > cache (or new tables marked as cache). So blocking committing the policy > on those issues makes no sense IMHO. That's right, I didn't argue for that either. But there should then be at least a PR with a patch that disables the work-arounds for policy breakers (the three decl-debug hash-maps), that if applied breaks bootstrap, so that the fact that there's still a real bug somewhere doesn't get lost. Ciao, Michael.
On 13/07/15 16:21, Michael Matz wrote: > Hi, > > On Mon, 13 Jul 2015, Tom de Vries wrote: > >>> Implementing multi-step maps or making the hashmaps non-caching >>> doesn't solve any of the above problems >> >> I'm not saying that making those hashmaps non-caching solves any of >> these problems. > > Ah, I didn't mean to imply this, I meant to imply that enforcing policy as > you do is a good thing because it finds bugs, and that the policy to be > enforced should be forbidding multi-step deps :) > >> I'm saying that it decouples fixing the policy (for which I have a >> patch) from fixing the issues that allow us to use these 3 as caches >> again (for which there are no patches yet). The advantage of having a >> policy in place is that we won't regress for tables still marked as >> cache (or new tables marked as cache). So blocking committing the policy >> on those issues makes no sense IMHO. > > That's right, I didn't argue for that either. Great, we're in agreement then :) > But there should then be at > least a PR with a patch that disables the work-arounds for policy breakers > (the three decl-debug hash-maps), that if applied breaks bootstrap, so > that the fact that there's still a real bug somewhere doesn't get lost. > Yep, there should be a PR to track these issues. And I made the down-grades for each cache a single patch, to make it easy to revert once we fix all the issues for one table. Now let's see if I can get approval for "Don't mark live recursively in gt_cleare_cache". Thanks, - Tom
Use iterative strategy for caches --- gcc/gengtype.c | 29 +++++++++++++++++---------- gcc/ggc-common.c | 26 +++++++++++++++++++++++- gcc/ggc.h | 2 +- gcc/hash-table.h | 61 ++++++++++++++++++++++++++++++++++++++++++++------------ 4 files changed, 93 insertions(+), 25 deletions(-) diff --git a/gcc/gengtype.c b/gcc/gengtype.c index 137e0ff..3e912e3 100644 --- a/gcc/gengtype.c +++ b/gcc/gengtype.c @@ -4197,7 +4197,10 @@ finish_cache_funcs (flist *flp) for (fli2 = flp; fli2; fli2 = fli2->next) if (fli2->started_p) { - oprintf (fli2->f, "}\n\n"); + oprintf (fli2->f, + " return clear_cnt;\n" + "}\n" + "\n"); } for (fli2 = flp; fli2 && base_files; fli2 = fli2->next) @@ -4209,14 +4212,18 @@ finish_cache_funcs (flist *flp) for (fnum = 0; bitmap != 0; fnum++, bitmap >>= 1) if (bitmap & 1) { - oprintf (base_files[fnum], "extern void gt_clear_caches_"); + oprintf (base_files[fnum], "extern unsigned int gt_clear_caches_"); put_mangled_filename (base_files[fnum], fli2->file); - oprintf (base_files[fnum], " ();\n"); + oprintf (base_files[fnum], " (unsigned int);\n"); } } for (size_t fnum = 0; base_files && fnum < num_lang_dirs; fnum++) - oprintf (base_files[fnum], "void\ngt_clear_caches ()\n{\n"); + oprintf (base_files[fnum], + "unsigned int\n" + "gt_clear_caches (unsigned int phase)\n" + "{\n" + " unsigned int clear_cnt = 0;\n"); for (fli2 = flp; fli2; fli2 = fli2->next) if (fli2->started_p) @@ -4229,15 +4236,17 @@ finish_cache_funcs (flist *flp) for (fnum = 0; base_files && bitmap != 0; fnum++, bitmap >>= 1) if (bitmap & 1) { - oprintf (base_files[fnum], " gt_clear_caches_"); + oprintf (base_files[fnum], " clear_cnt += gt_clear_caches_"); put_mangled_filename (base_files[fnum], fli2->file); - oprintf (base_files[fnum], " ();\n"); + oprintf (base_files[fnum], " (phase);\n"); } } for (size_t fnum = 0; base_files && fnum < num_lang_dirs; fnum++) { - oprintf (base_files[fnum], "}\n"); + oprintf (base_files[fnum], + " return clear_cnt;\n" + "}\n"); } } @@ -4658,12 +4667,12 @@ write_roots (pair_p variables, bool emit_pch) { fli->started_p = 1; - oprintf (f, "void\ngt_clear_caches_"); + oprintf (f, "unsigned int\ngt_clear_caches_"); put_mangled_filename (f, v->line.file); - oprintf (f, " ()\n{\n"); + oprintf (f, " (unsigned int phase)\n{\n unsigned int clear_cnt = 0;\n"); } - oprintf (f, " gt_cleare_cache (%s);\n", v->name); + oprintf (f, " clear_cnt += gt_cleare_cache (%s, phase);\n", v->name); } finish_cache_funcs (flp); diff --git a/gcc/ggc-common.c b/gcc/ggc-common.c index 5096837..110640c 100644 --- a/gcc/ggc-common.c +++ b/gcc/ggc-common.c @@ -100,7 +100,31 @@ ggc_mark_roots (void) if (ggc_protect_identifiers) ggc_mark_stringpool (); - gt_clear_caches (); + unsigned int clear_cnt, prev_clear_cnt; + unsigned int iteration = 0; + + /* Mark. */ + ++iteration; + gt_clear_caches (1); + + /* Count. */ + clear_cnt = gt_clear_caches (0); + + do + { + prev_clear_cnt = clear_cnt; + + /* Mark. */ + ++iteration; + gt_clear_caches (1); + + /* Count. */ + clear_cnt = gt_clear_caches (0); + } + while (clear_cnt != prev_clear_cnt); + + /* Clear. */ + gt_clear_caches (2); if (! ggc_protect_identifiers) ggc_purge_stringpool (); diff --git a/gcc/ggc.h b/gcc/ggc.h index ebc6a5d..eaf4236 100644 --- a/gcc/ggc.h +++ b/gcc/ggc.h @@ -54,7 +54,7 @@ extern int gt_pch_note_object (void *, void *, gt_note_pointers); extern void gt_pch_note_reorder (void *, void *, gt_handle_reorder); /* generated function to clear caches in gc memory. */ -extern void gt_clear_caches (); +extern unsigned int gt_clear_caches (unsigned int); /* Mark the object in the first parameter and anything it points to. */ typedef void (*gt_pointer_walker) (void *); diff --git a/gcc/hash-table.h b/gcc/hash-table.h index 12e0c96..2408953 100644 --- a/gcc/hash-table.h +++ b/gcc/hash-table.h @@ -491,7 +491,8 @@ private: template<typename T> friend void gt_pch_nx (hash_table<T> *, gt_pointer_operator, void *); - template<typename T> friend void gt_cleare_cache (hash_table<T> *); + template<typename T> friend unsigned int gt_cleare_cache (hash_table<T> *, + unsigned int phase); value_type *alloc_entries (size_t n CXX_MEM_STAT_INFO) const; value_type *find_empty_slot_for_expand (hashval_t); @@ -1038,23 +1039,57 @@ gt_pch_nx (hash_table<D> *h, gt_pointer_operator op, void *cookie) } template<typename H> -inline void -gt_cleare_cache (hash_table<H> *h) +inline unsigned int +gt_cleare_cache (hash_table<H> *h, unsigned int phase) { + unsigned int clear_cnt = 0; + extern void gt_ggc_mx (typename H::value_type &t); typedef hash_table<H> table; if (!h) - return; + return clear_cnt; - for (typename table::iterator iter = h->begin (); iter != h->end (); ++iter) - if (!table::is_empty (*iter) && !table::is_deleted (*iter)) - { - int res = H::keep_cache_entry (*iter); - if (res == 0) - h->clear_slot (&*iter); - else if (res != -1) - gt_ggc_mx (*iter); - } + typename table::iterator iter; + switch (phase) + { + case 0: + /* Phase 0: Count. */ + for (iter = h->begin (); iter != h->end (); ++iter) + if (!table::is_empty (*iter) && !table::is_deleted (*iter)) + { + int res = H::keep_cache_entry (*iter); + if (res == 0) + clear_cnt++; + } + break; + + case 1: + /* Phase 1: Mark. */ + for (iter = h->begin (); iter != h->end (); ++iter) + if (!table::is_empty (*iter) && !table::is_deleted (*iter)) + { + int res = H::keep_cache_entry (*iter); + if (res == 1) + gt_ggc_mx (*iter); + } + break; + + case 2: + /* Phase 2: Clear. */ + for (iter = h->begin (); iter != h->end (); ++iter) + if (!table::is_empty (*iter) && !table::is_deleted (*iter)) + { + int res = H::keep_cache_entry (*iter); + if (res == 0) + h->clear_slot (&*iter); + } + break; + + default: + gcc_unreachable (); + } + + return clear_cnt; } #endif /* TYPED_HASHTAB_H */ -- 1.9.1