diff mbox series

Be careful about comdat boundary in ICF (PR ipa/82352).

Message ID 2a3a06b3-489d-16ef-172a-e3b3bd3aca25@suse.cz
State New
Headers show
Series Be careful about comdat boundary in ICF (PR ipa/82352). | expand

Commit Message

Martin Liška Jan. 3, 2018, 11:26 a.m. UTC 
Hi.

This patch is follow-up of r246848. This time ICF creates an edge between 2 functions,
where one is inside a comdat group and second is not. I've got patch that is conservative
about the comdat groups (in_same_comdat_group_p).

Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

Ready to be installed?
Martin

gcc/ChangeLog:

2018-01-03  Martin Liska  <mliska@suse.cz>

	PR ipa/82352
	* ipa-icf.c (sem_function::merge): Do not cross comdat boundary.

gcc/testsuite/ChangeLog:

2018-01-03  Martin Liska  <mliska@suse.cz>

	PR ipa/82352
	* g++.dg/ipa/pr82352.C: New test.
---
 gcc/ipa-icf.c                      |  9 ++++
 gcc/testsuite/g++.dg/ipa/pr82352.C | 93 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/ipa/pr82352.C

Comments

Jan Hubicka Jan. 3, 2018, 1:24 p.m. UTC | #1 
> Hi.
> 
> This patch is follow-up of r246848. This time ICF creates an edge between 2 functions,
> where one is inside a comdat group and second is not. I've got patch that is conservative
> about the comdat groups (in_same_comdat_group_p).
> 
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
> 
> Ready to be installed?
> Martin
> 
> gcc/ChangeLog:
> 
> 2018-01-03  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/82352
> 	* ipa-icf.c (sem_function::merge): Do not cross comdat boundary.
> 
> gcc/testsuite/ChangeLog:
> 
> 2018-01-03  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/82352
> 	* g++.dg/ipa/pr82352.C: New test.
> ---
>  gcc/ipa-icf.c                      |  9 ++++
>  gcc/testsuite/g++.dg/ipa/pr82352.C | 93 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 102 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/ipa/pr82352.C
> 
> 

> diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
> index a8d3b800318..a56c7306201 100644
> --- a/gcc/ipa-icf.c
> +++ b/gcc/ipa-icf.c
> @@ -1113,6 +1113,15 @@ sem_function::merge (sem_item *alias_item)
>        return false;
>      }
>  
> +  if (!original->in_same_comdat_group_p (alias))
> +    {
> +      if (dump_file)
> +	fprintf (dump_file, "Not unifying; alias cannot be created; "
> +		 "across comdat group boundary\n\n");
> +
> +      return false;
> +    }

Wasn't we supposed to do the wrapper in this case?

Honza
Martin Liška Jan. 3, 2018, 1:36 p.m. UTC | #2 
On 01/03/2018 02:24 PM, Jan Hubicka wrote:
>> Hi.
>>
>> This patch is follow-up of r246848. This time ICF creates an edge between 2 functions,
>> where one is inside a comdat group and second is not. I've got patch that is conservative
>> about the comdat groups (in_same_comdat_group_p).
>>
>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>
>> Ready to be installed?
>> Martin
>>
>> gcc/ChangeLog:
>>
>> 2018-01-03  Martin Liska  <mliska@suse.cz>
>>
>> 	PR ipa/82352
>> 	* ipa-icf.c (sem_function::merge): Do not cross comdat boundary.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2018-01-03  Martin Liska  <mliska@suse.cz>
>>
>> 	PR ipa/82352
>> 	* g++.dg/ipa/pr82352.C: New test.
>> ---
>>  gcc/ipa-icf.c                      |  9 ++++
>>  gcc/testsuite/g++.dg/ipa/pr82352.C | 93 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 102 insertions(+)
>>  create mode 100644 gcc/testsuite/g++.dg/ipa/pr82352.C
>>
>>
> 
>> diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
>> index a8d3b800318..a56c7306201 100644
>> --- a/gcc/ipa-icf.c
>> +++ b/gcc/ipa-icf.c
>> @@ -1113,6 +1113,15 @@ sem_function::merge (sem_item *alias_item)
>>        return false;
>>      }
>>  
>> +  if (!original->in_same_comdat_group_p (alias))
>> +    {
>> +      if (dump_file)
>> +	fprintf (dump_file, "Not unifying; alias cannot be created; "
>> +		 "across comdat group boundary\n\n");
>> +
>> +      return false;
>> +    }
> 
> Wasn't we supposed to do the wrapper in this case?
> 
> Honza
> 

We attempt to do a wrapper, but even with wrapper we cannot introduce such call
crossing the boundary. Proper message should be probably:

"Not unifying; alias nor wrapper cannot be created; across comdat group boundary"

Martin
Jan Hubicka Jan. 3, 2018, 1:40 p.m. UTC | #3 
> >> +  if (!original->in_same_comdat_group_p (alias))
> >> +    {
> >> +      if (dump_file)
> >> +	fprintf (dump_file, "Not unifying; alias cannot be created; "
> >> +		 "across comdat group boundary\n\n");
> >> +
> >> +      return false;
> >> +    }
> > 
> > Wasn't we supposed to do the wrapper in this case?
> > 
> > Honza
> > 
> 
> We attempt to do a wrapper, but even with wrapper we cannot introduce such call
> crossing the boundary. Proper message should be probably:
> 
> "Not unifying; alias nor wrapper cannot be created; across comdat group boundary"

If the symbol we are calling is one exported from comdat, it shoud work fine
as long as we produce gimple thunk and it the symbol comdat exports.
Of course producing call from one comdat to another may close comdat loop
(which we handle elsewhere, so i assume original is comdat and alias is not)

I see in the PR is the split part of comdat which is comdat local, so perhaps
we want to check that original is not comdat local in addition to your current
check?

Honza
> 
> Martin
Martin Liška Jan. 3, 2018, 2:14 p.m. UTC | #4 
On 01/03/2018 02:40 PM, Jan Hubicka wrote:
>>>> +  if (!original->in_same_comdat_group_p (alias))
>>>> +    {
>>>> +      if (dump_file)
>>>> +	fprintf (dump_file, "Not unifying; alias cannot be created; "
>>>> +		 "across comdat group boundary\n\n");
>>>> +
>>>> +      return false;
>>>> +    }
>>>
>>> Wasn't we supposed to do the wrapper in this case?
>>>
>>> Honza
>>>
>>
>> We attempt to do a wrapper, but even with wrapper we cannot introduce such call
>> crossing the boundary. Proper message should be probably:
>>
>> "Not unifying; alias nor wrapper cannot be created; across comdat group boundary"
> 
> If the symbol we are calling is one exported from comdat, it shoud work fine
> as long as we produce gimple thunk and it the symbol comdat exports.
> Of course producing call from one comdat to another may close comdat loop
> (which we handle elsewhere, so i assume original is comdat and alias is not)
> 
> I see in the PR is the split part of comdat which is comdat local, so perhaps
> we want to check that original is not comdat local in addition to your current
> check?

Ok, you understand problematic of comdats more than me. I will test and install
patch with the check added.

Thanks,
Martin

> 
> Honza
>>
>> Martin
From 707e88333778b306f66c1bf683838b1a7bb4f737 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Wed, 3 Jan 2018 11:10:18 +0100
Subject: [PATCH] Be careful about comdat boundary in ICF (PR ipa/82352).

gcc/ChangeLog:

2018-01-03  Martin Liska  <mliska@suse.cz>

	PR ipa/82352
	* ipa-icf.c (sem_function::merge): Do not cross comdat boundary.

gcc/testsuite/ChangeLog:

2018-01-03  Martin Liska  <mliska@suse.cz>

	PR ipa/82352
	* g++.dg/ipa/pr82352.C: New test.
---
 gcc/ipa-icf.c                      | 11 +++++
 gcc/testsuite/g++.dg/ipa/pr82352.C | 93 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/ipa/pr82352.C

diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index a8d3b800318..e17c4292392 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -1113,6 +1113,17 @@ sem_function::merge (sem_item *alias_item)
       return false;
     }
 
+  if (!original->in_same_comdat_group_p (alias)
+      || original->comdat_local_p ())
+    {
+      if (dump_file)
+	fprintf (dump_file,
+		 "Not unifying; alias nor wrapper cannot be created; "
+		 "across comdat group boundary\n\n");
+
+      return false;
+    }
+
   /* See if original is in a section that can be discarded if the main
      symbol is not used.  */
 
diff --git a/gcc/testsuite/g++.dg/ipa/pr82352.C b/gcc/testsuite/g++.dg/ipa/pr82352.C
new file mode 100644
index 00000000000..c044345a486
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr82352.C
@@ -0,0 +1,93 @@
+// PR ipa/82352
+// { dg-do compile }
+// { dg-options "-O2" }
+
+typedef long unsigned int size_t;
+
+class A
+{
+public :
+  typedef enum { Zero = 0, One = 1 } tA;
+  A(tA a) { m_a = a; }
+
+private :
+  tA m_a;
+};
+
+class B
+{
+public :
+  void *operator new(size_t t) { return (void*)(42); };
+};
+
+class C
+{
+public:
+  virtual void ffff () = 0;
+};
+
+class D
+{
+ public :
+  virtual void g() = 0;
+  virtual void h() = 0;
+};
+
+template<class T> class IIII: public T, public D
+{
+public:
+ void ffff()
+ {
+   if (!m_i2) throw A(A::One);
+ };
+
+ void h()
+ {
+  if (m_i2) throw A(A::Zero);
+ }
+
+protected:
+ virtual void g()
+ {
+  if (m_i1 !=0) throw A(A::Zero);
+ };
+
+private :
+ int m_i1;
+ void *m_i2;
+};
+
+class E
+{
+private:
+    size_t m_e;
+    static const size_t Max;
+
+public:
+    E& i(size_t a, size_t b, size_t c)
+    {
+        if ((a > Max) || (c > Max)) throw A(A::Zero );
+        if (a + b > m_e) throw A(A::One );
+        return (*this);
+    }
+
+  inline E& j(const E &s)
+    {
+      return i(0,0,s.m_e);
+    }
+};
+
+class F : public C { };
+class G : public C { };
+class HHHH : public B, public F, public G { };
+
+void k()
+{
+    new IIII<HHHH>();
+}
+
+void l()
+{
+  E e1, e2;
+  e1.j(e2);
+}
Jakub Jelinek Jan. 4, 2018, 9:15 p.m. UTC | #5 
On Wed, Jan 03, 2018 at 03:14:10PM +0100, Martin Liška wrote:
> 2018-01-03  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/82352
> 	* g++.dg/ipa/pr82352.C: New test.
> ---
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ipa/pr82352.C
> @@ -0,0 +1,93 @@
> +// PR ipa/82352
> +// { dg-do compile }
> +// { dg-options "-O2" }
> +
> +typedef long unsigned int size_t;

...

This FAILs on i686-linux, fixed thusly, committed as obvious to trunk:

2018-01-04  Jakub Jelinek  <jakub@redhat.com>

	PR ipa/82352
	* g++.dg/ipa/pr82352.C (size_t): Define to __SIZE_TYPE__ instead of
	long unsigned int.

--- gcc/testsuite/g++.dg/ipa/pr82352.C.jj	2018-01-04 12:37:24.689487261 +0100
+++ gcc/testsuite/g++.dg/ipa/pr82352.C	2018-01-04 22:06:15.770654311 +0100
@@ -2,7 +2,7 @@
 // { dg-do compile }
 // { dg-options "-O2" }
 
-typedef long unsigned int size_t;
+typedef __SIZE_TYPE__ size_t;
 
 class A
 {


	Jakub
diff mbox series

Patch

diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
index a8d3b800318..a56c7306201 100644
--- a/gcc/ipa-icf.c
+++ b/gcc/ipa-icf.c
@@ -1113,6 +1113,15 @@  sem_function::merge (sem_item *alias_item)
       return false;
     }
 
+  if (!original->in_same_comdat_group_p (alias))
+    {
+      if (dump_file)
+	fprintf (dump_file, "Not unifying; alias cannot be created; "
+		 "across comdat group boundary\n\n");
+
+      return false;
+    }
+
   /* See if original is in a section that can be discarded if the main
      symbol is not used.  */
 
diff --git a/gcc/testsuite/g++.dg/ipa/pr82352.C b/gcc/testsuite/g++.dg/ipa/pr82352.C
new file mode 100644
index 00000000000..c044345a486
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ipa/pr82352.C
@@ -0,0 +1,93 @@ 
+// PR ipa/82352
+// { dg-do compile }
+// { dg-options "-O2" }
+
+typedef long unsigned int size_t;
+
+class A
+{
+public :
+  typedef enum { Zero = 0, One = 1 } tA;
+  A(tA a) { m_a = a; }
+
+private :
+  tA m_a;
+};
+
+class B
+{
+public :
+  void *operator new(size_t t) { return (void*)(42); };
+};
+
+class C
+{
+public:
+  virtual void ffff () = 0;
+};
+
+class D
+{
+ public :
+  virtual void g() = 0;
+  virtual void h() = 0;
+};
+
+template<class T> class IIII: public T, public D
+{
+public:
+ void ffff()
+ {
+   if (!m_i2) throw A(A::One);
+ };
+
+ void h()
+ {
+  if (m_i2) throw A(A::Zero);
+ }
+
+protected:
+ virtual void g()
+ {
+  if (m_i1 !=0) throw A(A::Zero);
+ };
+
+private :
+ int m_i1;
+ void *m_i2;
+};
+
+class E
+{
+private:
+    size_t m_e;
+    static const size_t Max;
+
+public:
+    E& i(size_t a, size_t b, size_t c)
+    {
+        if ((a > Max) || (c > Max)) throw A(A::Zero );
+        if (a + b > m_e) throw A(A::One );
+        return (*this);
+    }
+
+  inline E& j(const E &s)
+    {
+      return i(0,0,s.m_e);
+    }
+};
+
+class F : public C { };
+class G : public C { };
+class HHHH : public B, public F, public G { };
+
+void k()
+{
+    new IIII<HHHH>();
+}
+
+void l()
+{
+  E e1, e2;
+  e1.j(e2);
+}