* Fix verifier ICE on wrong comdat local flag [PR93347]
@ 2020-03-20 21:15 Jan Hubicka
0 siblings, 0 replies; only message in thread
From: Jan Hubicka @ 2020-03-20 21:15 UTC (permalink / raw)
To: 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 <hubicka@ucw.cz>
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 <hubicka@ucw.cz>
* 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 <cgraph_node *> (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<B*>(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<A*>(d)->f();
+ // CHECK: call i32 @_ZN5Test41BngEv
+ -static_cast<A&>(*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<A*>(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<A&>(*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<A*>(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<bar*>(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<B*>(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<RA*>(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<RA&>(*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<A *>(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<class T>
+ 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<int> *s = new S<int>;
+ s->foo1();
+ }
+}
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2020-03-20 21:15 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-20 21:15 Fix verifier ICE on wrong comdat local flag [PR93347] Jan Hubicka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).