From 72b3bc895f023bf451357659cfe96c966945bdf9 Mon Sep 17 00:00:00 2001 From: Jan Hubicka Date: Fri, 20 Mar 2020 22:06:24 +0100 Subject: [PATCH] Fix verifier ICE on wrong comdat local flag [PR93347] gcc/ChangeLog: 2020-03-20 Jan Hubicka PR ipa/93347 * cgraph.c (symbol_table::create_edge): Update calls_comdat_local flag. (cgraph_edge::redirect_callee): Move here; likewise. (cgraph_node::remove_callees): Update calls_comdat_local flag. (cgraph_node::verify_node): Verify that calls_comdat_local flag match reality. (cgraph_node::check_calls_comdat_local_p): New member function. * cgraph.h (cgraph_node::check_calls_comdat_local_p): Declare. (cgraph_edge::redirect_callee): Move offline. * ipa-fnsummary.c (compute_fn_summary): Do not compute calls_comdat_local flag here. * ipa-inline-transform.c (inline_call): Fix updating of calls_comdat_local flag. * ipa-split.c (split_function): Use true instead of 1 to set the flag. * symtab.c (symtab_node::add_to_same_comdat_group): Update calls_comdat_local flag. gcc/testsuite/ChangeLog: 2020-03-20 Jan Hubicka * g++.dg/torture/pr93347.C: New test. --- gcc/ChangeLog | 19 ++ gcc/cgraph.c | 64 ++++++- gcc/cgraph.h | 17 +- gcc/ipa-fnsummary.c | 4 - gcc/ipa-inline-transform.c | 9 +- gcc/ipa-split.c | 2 +- gcc/symtab.c | 11 ++ gcc/testsuite/ChangeLog | 5 + gcc/testsuite/g++.dg/torture/pr93347.C | 306 +++++++++++++++++++++++++++++++++ 9 files changed, 407 insertions(+), 30 deletions(-) create mode 100644 gcc/testsuite/g++.dg/torture/pr93347.C diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 967e454..7f00a13 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,22 @@ +2020-03-20 Jan Hubicka + + PR ipa/93347 + * cgraph.c (symbol_table::create_edge): Update calls_comdat_local flag. + (cgraph_edge::redirect_callee): Move here; likewise. + (cgraph_node::remove_callees): Update calls_comdat_local flag. + (cgraph_node::verify_node): Verify that calls_comdat_local flag match + reality. + (cgraph_node::check_calls_comdat_local_p): New member function. + * cgraph.h (cgraph_node::check_calls_comdat_local_p): Declare. + (cgraph_edge::redirect_callee): Move offline. + * ipa-fnsummary.c (compute_fn_summary): Do not compute + calls_comdat_local flag here. + * ipa-inline-transform.c (inline_call): Fix updating of + calls_comdat_local flag. + * ipa-split.c (split_function): Use true instead of 1 to set the flag. + * symtab.c (symtab_node::add_to_same_comdat_group): Update + calls_comdat_local flag. + 2020-03-20 Richard Biener * tree-vect-slp.c (vect_analyze_slp_instance): Dump SLP tree diff --git a/gcc/cgraph.c b/gcc/cgraph.c index b41dea1..6b780f8 100644 --- a/gcc/cgraph.c +++ b/gcc/cgraph.c @@ -557,7 +557,8 @@ cgraph_node::get_create (tree decl) } /* Mark ALIAS as an alias to DECL. DECL_NODE is cgraph node representing - the function body is associated with (not necessarily cgraph_node (DECL). */ + the function body is associated with + (not necessarily cgraph_node (DECL)). */ cgraph_node * cgraph_node::create_alias (tree alias, tree target) @@ -914,6 +915,10 @@ symbol_table::create_edge (cgraph_node *caller, cgraph_node *callee, else edge->in_polymorphic_cdtor = caller->thunk.thunk_p; + if (callee && symtab->state != LTO_STREAMING + && edge->callee->comdat_local_p ()) + edge->caller->calls_comdat_local = true; + return edge; } @@ -1341,6 +1346,34 @@ cgraph_edge::make_direct (cgraph_edge *edge, cgraph_node *callee) return edge; } +/* Redirect callee of the edge to N. The function does not update underlying + call expression. */ + +void +cgraph_edge::redirect_callee (cgraph_node *n) +{ + bool loc = callee->comdat_local_p (); + /* Remove from callers list of the current callee. */ + remove_callee (); + + /* Insert to callers list of the new callee. */ + set_callee (n); + + if (!inline_failed) + return; + if (!loc && n->comdat_local_p ()) + { + cgraph_node *to = caller->inlined_to ? caller->inlined_to : caller; + to->calls_comdat_local = true; + } + else if (loc && !n->comdat_local_p ()) + { + cgraph_node *to = caller->inlined_to ? caller->inlined_to : caller; + gcc_checking_assert (to->calls_comdat_local); + to->calls_comdat_local = to->check_calls_comdat_local_p (); + } +} + /* If necessary, change the function declaration in the call statement associated with E so that it corresponds to the edge callee. Speculations can be resolved in the process and EDGE can be removed and deallocated. @@ -1674,6 +1707,8 @@ cgraph_node::remove_callees (void) { cgraph_edge *e, *f; + calls_comdat_local = false; + /* It is sufficient to remove the edges from the lists of callers of the callees. The callee list of the node can be zapped with one assignment. */ @@ -3369,10 +3404,18 @@ cgraph_node::verify_node (void) error ("inline clone is forced to output"); error_found = true; } - if (calls_comdat_local && !same_comdat_group) + if (symtab->state != LTO_STREAMING) { - error ("calls_comdat_local is set outside of a comdat group"); - error_found = true; + if (calls_comdat_local && !same_comdat_group) + { + error ("calls_comdat_local is set outside of a comdat group"); + error_found = true; + } + if (!inlined_to && calls_comdat_local != check_calls_comdat_local_p ()) + { + error ("invalid calls_comdat_local flag"); + error_found = true; + } } if (DECL_IS_MALLOC (decl) && !POINTER_TYPE_P (TREE_TYPE (TREE_TYPE (decl)))) @@ -4044,6 +4087,19 @@ cgraph_edge::num_speculative_call_targets_p (void) return indirect_info ? indirect_info->num_speculative_call_targets : 0; } +/* Check if function calls comdat local. This is used to recompute + calls_comdat_local flag after function transformations. */ +bool +cgraph_node::check_calls_comdat_local_p () +{ + for (cgraph_edge *e = callees; e; e = e->next_callee) + if (e->inline_failed + ? e->callee->comdat_local_p () + : e->callee->check_calls_comdat_local_p ()) + return true; + return false; +} + /* A stashed copy of "symtab" for use by selftest::symbol_table_test. This needs to be a global so that it can be a GC root, and thus prevent the stashed copy from being garbage-collected if the GC runs diff --git a/gcc/cgraph.h b/gcc/cgraph.h index aa4cdc9..43de3b4 100644 --- a/gcc/cgraph.h +++ b/gcc/cgraph.h @@ -1326,6 +1326,10 @@ struct GTY((tag ("SYMTAB_FUNCTION"))) cgraph_node : public symtab_node /* Return true if this node represents a former, i.e. an expanded, thunk. */ inline bool former_thunk_p (void); + /* Check if function calls comdat local. This is used to recompute + calls_comdat_local flag after function transformations. */ + bool check_calls_comdat_local_p (); + /* Return true if function should be optimized for size. */ bool optimize_for_size_p (void); @@ -3298,19 +3302,6 @@ cgraph_edge::set_callee (cgraph_node *n) callee = n; } -/* Redirect callee of the edge to N. The function does not update underlying - call expression. */ - -inline void -cgraph_edge::redirect_callee (cgraph_node *n) -{ - /* Remove from callers list of the current callee. */ - remove_callee (); - - /* Insert to callers list of the new callee. */ - set_callee (n); -} - /* Return true when the edge represents a direct recursion. */ inline bool diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c index 059ea74..b411bc4 100644 --- a/gcc/ipa-fnsummary.c +++ b/gcc/ipa-fnsummary.c @@ -2944,10 +2944,6 @@ compute_fn_summary (struct cgraph_node *node, bool early) analyze_function_body (node, early); pop_cfun (); } - for (e = node->callees; e; e = e->next_callee) - if (e->callee->comdat_local_p ()) - break; - node->calls_comdat_local = (e != NULL); /* Inlining characteristics are maintained by the cgraph_mark_inline. */ size_info->size = size_info->self_size; diff --git a/gcc/ipa-inline-transform.c b/gcc/ipa-inline-transform.c index fa5015d..eed992d 100644 --- a/gcc/ipa-inline-transform.c +++ b/gcc/ipa-inline-transform.c @@ -504,14 +504,7 @@ inline_call (struct cgraph_edge *e, bool update_original, if (callee->calls_comdat_local) to->calls_comdat_local = true; else if (to->calls_comdat_local && comdat_local) - { - struct cgraph_edge *se = to->callees; - for (; se; se = se->next_callee) - if (se->inline_failed && se->callee->comdat_local_p ()) - break; - if (se == NULL) - to->calls_comdat_local = false; - } + to->calls_comdat_local = to->check_calls_comdat_local_p (); /* FIXME: This assert suffers from roundoff errors, disable it for GCC 5 and revisit it after conversion to sreals in GCC 6. diff --git a/gcc/ipa-split.c b/gcc/ipa-split.c index 87a0989..973e72c 100644 --- a/gcc/ipa-split.c +++ b/gcc/ipa-split.c @@ -1363,7 +1363,7 @@ split_function (basic_block return_bb, class split_point *split_point, { /* TODO: call is versionable if we make sure that all callers are inside of a comdat group. */ - cur_node->calls_comdat_local = 1; + cur_node->calls_comdat_local = true; node->add_to_same_comdat_group (cur_node); } diff --git a/gcc/symtab.c b/gcc/symtab.c index a879c09..3022acf 100644 --- a/gcc/symtab.c +++ b/gcc/symtab.c @@ -473,6 +473,17 @@ symtab_node::add_to_same_comdat_group (symtab_node *old_node) ; n->same_comdat_group = this; } + + cgraph_node *n; + if (comdat_local_p () + && (n = dyn_cast (this)) != NULL) + { + for (cgraph_edge *e = n->callers; e; e = e->next_caller) + if (e->caller->inlined_to) + e->caller->inlined_to->calls_comdat_local = true; + else + e->caller->calls_comdat_local = true; + } } /* Dissolve the same_comdat_group list in which NODE resides. */ diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index 76b93b5..50c42e2 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2020-03-20 Jan Hubicka + + PR ipa/93347 + * g++.dg/torture/pr93347.C: New test. + 2020-03-20 Patrick Palka PR c++/69694 diff --git a/gcc/testsuite/g++.dg/torture/pr93347.C b/gcc/testsuite/g++.dg/torture/pr93347.C new file mode 100644 index 0000000..3b5cc26 --- /dev/null +++ b/gcc/testsuite/g++.dg/torture/pr93347.C @@ -0,0 +1,306 @@ +// { dg-additional-options "--param early-inlining-insns=3 --param ipa-cp-eval-threshold=100" } + +namespace Test1 { + struct A { + virtual int f() final; + }; + + // CHECK-LABEL: define i32 @_ZN5Test11fEPNS_1AE + int f(A *a) { + // CHECK: call i32 @_ZN5Test11A1fEv + return a->f(); + } +} + +namespace Test2 { + struct A final { + virtual int f(); + }; + + // CHECK-LABEL: define i32 @_ZN5Test21fEPNS_1AE + int f(A *a) { + // CHECK: call i32 @_ZN5Test21A1fEv + return a->f(); + } +} + +namespace Test2a { + struct A { + virtual ~A() final {} + virtual int f(); + }; + + // CHECK-LABEL: define i32 @_ZN6Test2a1fEPNS_1AE + int f(A *a) { + // CHECK: call i32 @_ZN6Test2a1A1fEv + return a->f(); + } +} + + +namespace Test3 { + struct A { + virtual int f(); }; + + struct B final : A { }; + + // CHECK-LABEL: define i32 @_ZN5Test31fEPNS_1BE + int f(B *b) { + // CHECK: call i32 @_ZN5Test31A1fEv + return b->f(); + } + + // CHECK-LABEL: define i32 @_ZN5Test31fERNS_1BE + int f(B &b) { + // CHECK: call i32 @_ZN5Test31A1fEv + return b.f(); + } + + // CHECK-LABEL: define i32 @_ZN5Test31fEPv + int f(void *v) { + // CHECK: call i32 @_ZN5Test31A1fEv + return static_cast(v)->f(); + } +} + +namespace Test4 { + struct A { + virtual void f(); + virtual int operator-(); + }; + + struct B final : A { + virtual void f(); + virtual int operator-(); + }; + + // CHECK-LABEL: define void @_ZN5Test41fEPNS_1BE + void f(B* d) { + // CHECK: call void @_ZN5Test41B1fEv + static_cast(d)->f(); + // CHECK: call i32 @_ZN5Test41BngEv + -static_cast(*d); + } +} + +namespace Test5 { + struct A { + virtual void f(); + virtual int operator-(); + }; + + struct B : A { + virtual void f(); + virtual int operator-(); + }; + + struct C final : B { + }; + + // CHECK-LABEL: define void @_ZN5Test51fEPNS_1CE + void f(C* d) { + // FIXME: It should be possible to devirtualize this case, but that is + // not implemented yet. + // CHECK: getelementptr + // CHECK-NEXT: %[[FUNC:.*]] = load + // CHECK-NEXT: call void %[[FUNC]] + static_cast(d)->f(); + } + // CHECK-LABEL: define void @_ZN5Test53fopEPNS_1CE + void fop(C* d) { + // FIXME: It should be possible to devirtualize this case, but that is + // not implemented yet. + // CHECK: getelementptr + // CHECK-NEXT: %[[FUNC:.*]] = load + // CHECK-NEXT: call i32 %[[FUNC]] + -static_cast(*d); + } +} + +namespace Test6 { + struct A { + virtual ~A(); + }; + + struct B : public A { + virtual ~B(); + }; + + struct C { + virtual ~C(); + }; + + struct D final : public C, public B { + }; + + // CHECK-LABEL: define void @_ZN5Test61fEPNS_1DE + void f(D* d) { + // CHECK: call void @_ZN5Test61DD1Ev + static_cast(d)->~A(); + } +} + +namespace Test7 { + struct foo { + virtual void g() {} + }; + + struct bar { + virtual int f() { return 0; } + }; + + struct zed final : public foo, public bar { + int z; + virtual int f() {return z;} + }; + + // CHECK-LABEL: define i32 @_ZN5Test71fEPNS_3zedE + int f(zed *z) { + // CHECK: alloca + // CHECK-NEXT: store + // CHECK-NEXT: load + // CHECK-NEXT: call i32 @_ZN5Test73zed1fEv + // CHECK-NEXT: ret + return static_cast(z)->f(); + } +} + +namespace Test8 { + struct A { virtual ~A() {} }; + struct B { + int b; + virtual int foo() { return b; } + }; + struct C final : A, B { }; + // CHECK-LABEL: define i32 @_ZN5Test84testEPNS_1CE + int test(C *c) { + // CHECK: %[[THIS:.*]] = phi + // CHECK-NEXT: call i32 @_ZN5Test81B3fooEv(%"struct.Test8::B"* %[[THIS]]) + return static_cast(c)->foo(); + } +} + +namespace Test9 { + struct A { + int a; + }; + struct B { + int b; + }; + struct C : public B, public A { + }; + struct RA { + virtual A *f() { + return 0; + } + virtual A *operator-() { + return 0; + } + }; + struct RC final : public RA { + virtual C *f() { + C *x = new C(); + x->a = 1; + x->b = 2; + return x; + } + virtual C *operator-() { + C *x = new C(); + x->a = 1; + x->b = 2; + return x; + } + }; + // CHECK: define {{.*}} @_ZN5Test91fEPNS_2RCE + A *f(RC *x) { + // FIXME: It should be possible to devirtualize this case, but that is + // not implemented yet. + // CHECK: load + // CHECK: bitcast + // CHECK: [[F_PTR_RA:%.+]] = bitcast + // CHECK: [[VTABLE:%.+]] = load {{.+}} [[F_PTR_RA]] + // CHECK: [[VFN:%.+]] = getelementptr inbounds {{.+}} [[VTABLE]], i{{[0-9]+}} 0 + // CHECK-NEXT: %[[FUNC:.*]] = load {{.+}} [[VFN]] + // CHECK-NEXT: = call {{.*}} %[[FUNC]] + return static_cast(x)->f(); + } + // CHECK: define {{.*}} @_ZN5Test93fopEPNS_2RCE + A *fop(RC *x) { + // FIXME: It should be possible to devirtualize this case, but that is + // not implemented yet. + // CHECK: load + // CHECK: bitcast + // CHECK: [[F_PTR_RA:%.+]] = bitcast + // CHECK: [[VTABLE:%.+]] = load {{.+}} [[F_PTR_RA]] + // CHECK: [[VFN:%.+]] = getelementptr inbounds {{.+}} [[VTABLE]], i{{[0-9]+}} 1 + // CHECK-NEXT: %[[FUNC:.*]] = load {{.+}} [[VFN]] + // CHECK-NEXT: = call {{.*}} %[[FUNC]] + return -static_cast(*x); + } +} + +namespace Test10 { + struct A { + virtual int f(); + }; + + struct B : A { + int f() final; + }; + + // CHECK-LABEL: define i32 @_ZN6Test101fEPNS_1BE + int f(B *b) { + // CHECK: call i32 @_ZN6Test101B1fEv + return static_cast(b)->f(); + } +} + +namespace Test11 { + // Check that the definitions of Derived's operators are emitted. + + // CHECK-LABEL: define linkonce_odr void @_ZN6Test111SIiE4foo1Ev( + // CHECK: call void @_ZN6Test111SIiE7DerivedclEv( + // CHECK: call zeroext i1 @_ZN6Test111SIiE7DerivedeqERKNS_4BaseE( + // CHECK: call zeroext i1 @_ZN6Test111SIiE7DerivedntEv( + // CHECK: call dereferenceable(4) %"class.Test11::Base"* @_ZN6Test111SIiE7DerivedixEi( + // CHECK: define linkonce_odr void @_ZN6Test111SIiE7DerivedclEv( + // CHECK: define linkonce_odr zeroext i1 @_ZN6Test111SIiE7DerivedeqERKNS_4BaseE( + // CHECK: define linkonce_odr zeroext i1 @_ZN6Test111SIiE7DerivedntEv( + // CHECK: define linkonce_odr dereferenceable(4) %"class.Test11::Base"* @_ZN6Test111SIiE7DerivedixEi( + class Base { + public: + virtual void operator()() {} + virtual bool operator==(const Base &other) { return false; } + virtual bool operator!() { return false; } + virtual Base &operator[](int i) { return *this; } + }; + + template + struct S { + class Derived final : public Base { + public: + void operator()() override {} + bool operator==(const Base &other) override { return true; } + bool operator!() override { return true; } + Base &operator[](int i) override { return *this; } + }; + + Derived *ptr = nullptr, *ptr2 = nullptr; + + void foo1() { + if (ptr && ptr2) { + // These calls get devirtualized. Linkage fails if the definitions of + // the called functions are not emitted. + (*ptr)(); + (void)(*ptr == *ptr2); + (void)(!(*ptr)); + (void)((*ptr)[1]); + } + } + }; + + void foo2() { + S *s = new S; + s->foo1(); + } +} -- 2.7.4