From patchwork Fri Mar 20 21:15:49 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Hubicka X-Patchwork-Id: 1259250 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=8.43.85.97; helo=sourceware.org; envelope-from=gcc-patches-bounces@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ucw.cz Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 48kc5C3t0kz9sQt for ; Sat, 21 Mar 2020 08:15:57 +1100 (AEDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 2988C394741E; Fri, 20 Mar 2020 21:15:55 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from nikam.ms.mff.cuni.cz (nikam.ms.mff.cuni.cz [195.113.20.16]) by sourceware.org (Postfix) with ESMTPS id 862A83948A41 for ; Fri, 20 Mar 2020 21:15:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 862A83948A41 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=ucw.cz Authentication-Results: sourceware.org; spf=none smtp.mailfrom=hubicka@kam.mff.cuni.cz Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id 66771280404; Fri, 20 Mar 2020 22:15:49 +0100 (CET) Date: Fri, 20 Mar 2020 22:15:49 +0100 From: Jan Hubicka To: gcc-patches@gcc.gnu.org Subject: Fix verifier ICE on wrong comdat local flag [PR93347] Message-ID: <20200320211549.GA16094@kam.mff.cuni.cz> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.10.1 (2018-07-13) X-Spam-Status: No, score=-25.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces@gcc.gnu.org Sender: "Gcc-patches" Hi, this PR is about the new call_comdat_local_p verifier complaining on flag being set outside of comdat group. We generally are quite lazy about keeping the flag up to date that makes it hard to verify and leads to odd bugs. Since comdat locals became more frequent with IPA passes producing them in some cases I decided to go ahead and make the flag up-to-date throughout the compilation. I hope there won't be too many side corners to look at. Bootstrapped/regtested x86_64-linux, comitted. 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. diff --git a/gcc/cgraph.c b/gcc/cgraph.c index b41dea1fcca..6b780f80eb3 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 aa4cdc95506..43de3b4a8ac 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 059ea74278f..b411bc4d660 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 fa5015d386b..eed992d314d 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 87a098913b4..973e72cea04 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 a879c095a1a..3022acfc1ea 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/g++.dg/torture/pr93347.C b/gcc/testsuite/g++.dg/torture/pr93347.C new file mode 100644 index 00000000000..3b5cc265a1a --- /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(); + } +}