Message ID | 20191113210823.wkznroh6m2vr6nul@kam.mff.cuni.cz |
---|---|
State | New |
Headers | show |
Series | Fix ICE when inlining into function containing polymorphic call | expand |
On Wed, Nov 13, 2019 at 10:08:23PM +0100, Jan Hubicka wrote: > PR c++/92421 > * ipa-prop.c (update_indirect_edges_after_inlining): > Mark parameter as used. > * ipa-inline.c (recursive_inlining): Reset node cache > after inlining. > (inline_small_functions): Remove checking ifdef. > * ipa-inline-analysis.c (do_estimate_edge_time): Verify > cache consistency. > * g++.dg/torture/pr92421.C: New testcase. The testcase FAILs everywhere: FAIL: g++.dg/torture/pr92421.C -O0 (test for excess errors) FAIL: g++.dg/torture/pr92421.C -O1 (test for excess errors) FAIL: g++.dg/torture/pr92421.C -O2 (test for excess errors) ... FAIL: g++.dg/torture/pr92421.C -O0 (test for excess errors) Excess errors: /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:112:3: warning: no return statement in function returning non-void [-Wreturn-type] /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:121:3: warning: no return statement in function returning non-void [-Wreturn-type] /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:166:10: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:166:37: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:172:10: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:174:1: warning: no return statement in function returning non-void [-Wreturn-type] /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:60:10: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:60:34: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:62:10: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:62:34: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:71:8: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:71:32: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:47:7: warning: 'q::bb(char*, long int, char*, long int)::bf::bf(long int)' used but never defined I've fixed all the -Wwrite-strings warnings, all but one -Wreturn-type warnings (the one spot left made the ICE go away with unfixed g++ at -O3), the undefined bf ctor warning, tested on x86_64-linux, verified unfixed g++ still ICEs, committed to trunk as obvious: 2019-11-14 Jakub Jelinek <jakub@redhat.com> PR ipa/92421 * g++.dg/torture/pr92421.C: Add -Wno-return-type to dg-additional-options. Avoid -Wwrite-string warnings, most of -Wreturn-type warnings, define bf ctor. Use struct instead of class with public: at the start. --- gcc/testsuite/g++.dg/torture/pr92421.C.jj 2019-11-14 01:56:41.682884599 +0100 +++ gcc/testsuite/g++.dg/torture/pr92421.C 2019-11-14 01:55:26.968004306 +0100 @@ -1,55 +1,49 @@ -/* { dg-do compile } */ +// PR ipa/92421 +// { dg-do compile } +// { dg-additional-options "-Wno-return-type" } + typedef long a; void *b, *c; template <typename, typename> class d {}; template <typename e, typename f> bool operator!=(d<e, f>, d<e, f>); -class g { -public: - g(char *); +struct g { + g(const char *); }; -class j { -public: +struct j { j(); void h(); void i(); void aj(); }; -class m { -public: +struct m { m(bool); }; -class n { -public: +struct n { operator a(); }; -class o { -public: +struct o { long am(); }; -class H { -public: +struct H { class p {}; virtual bool accept(const char *, unsigned long, p *, bool); }; -class q : H { -public: - class r { - public: +struct q : H { + struct r { enum at { au, av, aw }; }; enum { ax }; - virtual void ay(char *, int, const char *, r::at, char *); + virtual void ay(const char *, int, const char *, r::at, const char *); virtual bool az(const g &, unsigned = ax); virtual bool ba(const int &, p *, bool); void bb(char *bc, long bd, char *, long be) { - class bf : public p { - public: - bf(long); + struct bf : public p { + bf(long) {} } bg(be); accept(bc, bd, &bg, true); } }; -class s { +struct s { q *bi; bool bj(); }; @@ -109,6 +103,7 @@ template <class bk> class t : q { while (kit != df) ; cx(); + return false; } bool az(const g &, unsigned) { t dj; @@ -157,8 +152,7 @@ template <class bk> class t : q { O db[6]; bool bp; }; -class w : q { -public: +struct w : q { void dn(); bool l() { m(true); @@ -171,4 +165,5 @@ public: bool s::bj() { bi->az(""); new t<w>; + return false; } Jakub
> On Wed, Nov 13, 2019 at 10:08:23PM +0100, Jan Hubicka wrote: > > PR c++/92421 > > * ipa-prop.c (update_indirect_edges_after_inlining): > > Mark parameter as used. > > * ipa-inline.c (recursive_inlining): Reset node cache > > after inlining. > > (inline_small_functions): Remove checking ifdef. > > * ipa-inline-analysis.c (do_estimate_edge_time): Verify > > cache consistency. > > * g++.dg/torture/pr92421.C: New testcase. > > The testcase FAILs everywhere: > FAIL: g++.dg/torture/pr92421.C -O0 (test for excess errors) > FAIL: g++.dg/torture/pr92421.C -O1 (test for excess errors) > FAIL: g++.dg/torture/pr92421.C -O2 (test for excess errors) > ... > FAIL: g++.dg/torture/pr92421.C -O0 (test for excess errors) > Excess errors: > /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:112:3: warning: no return statement in function returning non-void [-Wreturn-type] > /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:121:3: warning: no return statement in function returning non-void [-Wreturn-type] > /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:166:10: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] > /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:166:37: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] > /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:172:10: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] > /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:174:1: warning: no return statement in function returning non-void [-Wreturn-type] > /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:60:10: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] > /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:60:34: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] > /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:62:10: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] > /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:62:34: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] > /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:71:8: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] > /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:71:32: warning: ISO C++ forbids converting a string constant to 'char*' [-Wwrite-strings] > /usr/src/gcc/gcc/testsuite/g++.dg/torture/pr92421.C:47:7: warning: 'q::bb(char*, long int, char*, long int)::bf::bf(long int)' used but never defined > > I've fixed all the -Wwrite-strings warnings, all but one > -Wreturn-type warnings (the one spot left made the ICE go away > with unfixed g++ at -O3), the undefined bf ctor warning, tested on > x86_64-linux, verified unfixed g++ still ICEs, committed to trunk > as obvious: > > 2019-11-14 Jakub Jelinek <jakub@redhat.com> > > PR ipa/92421 > * g++.dg/torture/pr92421.C: Add -Wno-return-type to > dg-additional-options. Avoid -Wwrite-string warnings, most of > -Wreturn-type warnings, define bf ctor. Use struct instead of class > with public: at the start. Thanks a lot and sorry for the breakage. Honza
Hi, On Wed, Nov 13 2019, Jan Hubicka wrote: > Hi, > the testcase causes inline context cache to go out of sync because I > forgot to update used flags of parameters in one path of > update_indirect_edges_after_inlining. > > While debugging it I also added better consistency check to > ipa-inline-analysis and turned ipa-inline test from ifdef to -fchecking. > This uncovered yet another missed upate in recursive inliner. > > Bootstrapped/regtested x86_64-linux, comitted. > > PR c++/92421 > * ipa-prop.c (update_indirect_edges_after_inlining): > Mark parameter as used. > * ipa-inline.c (recursive_inlining): Reset node cache > after inlining. > (inline_small_functions): Remove checking ifdef. > * ipa-inline-analysis.c (do_estimate_edge_time): Verify > cache consistency. > * g++.dg/torture/pr92421.C: New testcase. > Index: ipa-prop.c > =================================================================== > --- ipa-prop.c (revision 278151) > +++ ipa-prop.c (working copy) > @@ -3537,6 +3537,11 @@ update_indirect_edges_after_inlining (st > if (ici->polymorphic > && !ipa_get_jf_ancestor_type_preserved (jfunc)) > ici->vptr_changed = true; > + ipa_set_param_used_by_indirect_call (new_root_info, > + ici->param_index, true); > + if (ici->polymorphic) > + ipa_set_param_used_by_polymorphic_call (new_root_info, > + ici->param_index, true); > } Interesting, you have this exact hunk already in the patch introducing the new param flags (message id id:20191103224712.ndzyxu6cn3jt3enu@kam.mff.cuni.cz or https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00077.html). I did actually check it was there, even if only yesterday evening, but I did :-) And I can also see the code already in my Monday checkout (r278047). So I guess you must have actually removed it by accident in the meantime? Martin
> Hi, > > On Wed, Nov 13 2019, Jan Hubicka wrote: > > Hi, > > the testcase causes inline context cache to go out of sync because I > > forgot to update used flags of parameters in one path of > > update_indirect_edges_after_inlining. > > > > While debugging it I also added better consistency check to > > ipa-inline-analysis and turned ipa-inline test from ifdef to -fchecking. > > This uncovered yet another missed upate in recursive inliner. > > > > Bootstrapped/regtested x86_64-linux, comitted. > > > > PR c++/92421 > > * ipa-prop.c (update_indirect_edges_after_inlining): > > Mark parameter as used. > > * ipa-inline.c (recursive_inlining): Reset node cache > > after inlining. > > (inline_small_functions): Remove checking ifdef. > > * ipa-inline-analysis.c (do_estimate_edge_time): Verify > > cache consistency. > > * g++.dg/torture/pr92421.C: New testcase. > > Index: ipa-prop.c > > =================================================================== > > --- ipa-prop.c (revision 278151) > > +++ ipa-prop.c (working copy) > > @@ -3537,6 +3537,11 @@ update_indirect_edges_after_inlining (st > > if (ici->polymorphic > > && !ipa_get_jf_ancestor_type_preserved (jfunc)) > > ici->vptr_changed = true; > > + ipa_set_param_used_by_indirect_call (new_root_info, > > + ici->param_index, true); > > + if (ici->polymorphic) > > + ipa_set_param_used_by_polymorphic_call (new_root_info, > > + ici->param_index, true); > > } > > > > Interesting, you have this exact hunk already in the patch introducing > the new param flags (message id > id:20191103224712.ndzyxu6cn3jt3enu@kam.mff.cuni.cz or > https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00077.html). I did > actually check it was there, even if only yesterday evening, but I did :-) > > And I can also see the code already in my Monday checkout (r278047). So > I guess you must have actually removed it by accident in the meantime? Well, it is there twice - once for PASS_THROUGH and here for ANCESTOR. But it is quite possible that I had both (since I also had verification at the place when originally doing the patch) and lost it on way to mainline. Honza > > Martin
On Thu, Nov 14 2019, Jan Hubicka wrote: >> On Wed, Nov 13 2019, Jan Hubicka wrote: >> > Hi, >> > the testcase causes inline context cache to go out of sync because I >> > forgot to update used flags of parameters in one path of >> > update_indirect_edges_after_inlining. >> > >> > While debugging it I also added better consistency check to >> > ipa-inline-analysis and turned ipa-inline test from ifdef to -fchecking. >> > This uncovered yet another missed upate in recursive inliner. >> > >> > Bootstrapped/regtested x86_64-linux, comitted. >> > >> > PR c++/92421 >> > * ipa-prop.c (update_indirect_edges_after_inlining): >> > Mark parameter as used. >> > * ipa-inline.c (recursive_inlining): Reset node cache >> > after inlining. >> > (inline_small_functions): Remove checking ifdef. >> > * ipa-inline-analysis.c (do_estimate_edge_time): Verify >> > cache consistency. >> > * g++.dg/torture/pr92421.C: New testcase. >> > Index: ipa-prop.c >> > =================================================================== >> > --- ipa-prop.c (revision 278151) >> > +++ ipa-prop.c (working copy) >> > @@ -3537,6 +3537,11 @@ update_indirect_edges_after_inlining (st >> > if (ici->polymorphic >> > && !ipa_get_jf_ancestor_type_preserved (jfunc)) >> > ici->vptr_changed = true; >> > + ipa_set_param_used_by_indirect_call (new_root_info, >> > + ici->param_index, true); >> > + if (ici->polymorphic) >> > + ipa_set_param_used_by_polymorphic_call (new_root_info, >> > + ici->param_index, true); >> > } >> >> >> >> Interesting, you have this exact hunk already in the patch introducing >> the new param flags (message id >> id:20191103224712.ndzyxu6cn3jt3enu@kam.mff.cuni.cz or >> https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00077.html). I did >> actually check it was there, even if only yesterday evening, but I did :-) >> >> And I can also see the code already in my Monday checkout (r278047). So >> I guess you must have actually removed it by accident in the meantime? > > Well, it is there twice - once for PASS_THROUGH and here for ANCESTOR. > But it is quite possible that I had both (since I also had verification > at the place when originally doing the patch) and lost it on way to > mainline. > Oh, I see. That explains it then. Thanks, Martin
Index: ipa-prop.c =================================================================== --- ipa-prop.c (revision 278151) +++ ipa-prop.c (working copy) @@ -3537,6 +3537,11 @@ update_indirect_edges_after_inlining (st if (ici->polymorphic && !ipa_get_jf_ancestor_type_preserved (jfunc)) ici->vptr_changed = true; + ipa_set_param_used_by_indirect_call (new_root_info, + ici->param_index, true); + if (ici->polymorphic) + ipa_set_param_used_by_polymorphic_call (new_root_info, + ici->param_index, true); } } else Index: ipa-inline.c =================================================================== --- ipa-inline.c (revision 278151) +++ ipa-inline.c (working copy) @@ -1633,6 +1633,7 @@ recursive_inlining (struct cgraph_edge * } inline_call (curr, false, new_edges, &overall_size, true); + reset_node_cache (node); lookup_recursive_calls (node, curr->callee, &heap); n++; } @@ -1982,11 +1983,10 @@ inline_small_functions (void) if (!edge->inline_failed || !edge->callee->analyzed) continue; -#if CHECKING_P /* Be sure that caches are maintained consistent. This check is affected by scaling roundoff errors when compiling for IPA this we skip it in that case. */ - if (!edge->callee->count.ipa_p () + if (flag_checking && !edge->callee->count.ipa_p () && (!max_count.initialized_p () || !max_count.nonzero_p ())) { sreal cached_badness = edge_badness (edge, false); @@ -1997,6 +1997,9 @@ inline_small_functions (void) if (edge_growth_cache != NULL) edge_growth_cache->remove (edge); + reset_node_cache (edge->caller->inlined_to + ? edge->caller->inlined_to + : edge->caller); gcc_assert (old_size_est == estimate_edge_size (edge)); gcc_assert (old_time_est == estimate_edge_time (edge)); /* FIXME: @@ -2021,9 +2024,6 @@ inline_small_functions (void) } else current_badness = edge_badness (edge, false); -#else - current_badness = edge_badness (edge, false); -#endif if (current_badness != badness) { if (edge_heap.min () && current_badness > edge_heap.min_key ()) Index: ipa-inline-analysis.c =================================================================== --- ipa-inline-analysis.c (revision 278151) +++ ipa-inline-analysis.c (working copy) @@ -210,6 +210,19 @@ do_estimate_edge_time (struct cgraph_edg time = e->entry.time; nonspec_time = e->entry.nonspec_time; hints = e->entry.hints; + if (flag_checking) + { + sreal chk_time, chk_nonspec_time; + int chk_size, chk_min_size; + + ipa_hints chk_hints; + ctx.estimate_size_and_time (&chk_size, &chk_min_size, + &chk_time, &chk_nonspec_time, + &chk_hints); + gcc_assert (chk_size == size && chk_time == time + && chk_nonspec_time == nonspec_time + && chk_hints == hints); + } } else { Index: testsuite/g++.dg/torture/pr92421.C =================================================================== --- testsuite/g++.dg/torture/pr92421.C (nonexistent) +++ testsuite/g++.dg/torture/pr92421.C (working copy) @@ -0,0 +1,174 @@ +/* { dg-do compile } */ +typedef long a; +void *b, *c; +template <typename, typename> class d {}; +template <typename e, typename f> bool operator!=(d<e, f>, d<e, f>); +class g { +public: + g(char *); +}; +class j { +public: + j(); + void h(); + void i(); + void aj(); +}; +class m { +public: + m(bool); +}; +class n { +public: + operator a(); +}; +class o { +public: + long am(); +}; +class H { +public: + class p {}; + virtual bool accept(const char *, unsigned long, p *, bool); +}; +class q : H { +public: + class r { + public: + enum at { au, av, aw }; + }; + enum { ax }; + virtual void ay(char *, int, const char *, r::at, char *); + virtual bool az(const g &, unsigned = ax); + virtual bool ba(const int &, p *, bool); + void bb(char *bc, long bd, char *, long be) { + class bf : public p { + public: + bf(long); + } bg(be); + accept(bc, bd, &bg, true); + } +}; +class s { + q *bi; + bool bj(); +}; +template <class bk> class t : q { + bool accept(const char *, unsigned long bd, p *bg, bool) { + bool k(bp || bq), cl = false, err = false; + if (br) + ay("", 1, __func__, r::au, ""); + if (bs) + ay("", 6, __func__, r::av, ""); + char bt[1], cd[1]; + long bu = sizeof(int) + bd, ce = sizeof(L) + bd; + char *bw = bu > sizeof(bt) ? new char : bt, + *cf = ce > sizeof(cd) ? new char : cd; + __builtin___memcpy_chk(b, c, bd, 0); + a by[1]; + int bz = 0; + u cb = *cc((int *)bw, true, by, &bz); + ay("", 1, __func__, r::aw, ""); + if (bw != bt) + delete bw; + __builtin___memcpy_chk(b, c, bd, 0); + cb.ch.i(); + bool ci = cj((L *)cf, bg); + bool atran = bq && bp && cb.ck; + if (atran && !ci && cm(&cb)) + if (cn > co) { + int cp = cb.cq % 6; + v cs = *(ct + cp); + if (cu(&cs)) + cl = true; + } + if (ci) + if (k) + cv.aj(); + cv.h(); + b = cc((int *)bw, false, by, &bz); + if (b) + if (cw(&cb, by, bz)) + if (atran && bp && cx()) + cv.aj(); + if (cl) + if (k) + cv.aj(); + cv.h(); + int cp = cb.cq % 6; + v cs = *(ct + cp); + if (cy()) + err = true; + O da = *(db + cp); + if (da.dc->am() > cs.dc->am() + cs.dd->am() + 1 && de(&da)) + cv.aj(); + return !err; + } + bool ba(const int &, p *, bool) { + d<int, int> kit, df; + while (kit != df) + ; + cx(); + } + bool az(const g &, unsigned) { + t dj; + int cur; + while (cur) { + int dk, dl; + char dbuf; + dj.bb(&dbuf, dl, &dbuf, dk); + } + } + struct L {}; + struct u { + j ch; + a cq; + bool ck; + }; + struct v { + o *dd; + o *dc; + }; + struct O { + o *dc; + }; + bool cy(); + bool cu(v *); + bool cj(L *, p *); + bool de(O *); + u *cc(int *, bool, a *, int *); + bool cw(u *, a *, int); + bool cx() { + dm.dn(); + bool err = false; + if (dm.l()) + err = true; + return !err; + } + bool cm(u *); + j cv; + int br; + bool bs; + bool bq; + bk dm; + a co; + n cn; + v ct[6]; + O db[6]; + bool bp; +}; +class w : q { +public: + void dn(); + bool l() { + m(true); + if (br) + ay("", 1087, __func__, r::au, ""); + return false; + } + int br; +}; +bool s::bj() { + bi->az(""); + new t<w>; +}