[PATCH] ipa/96291: don't crash on unoptimized lto functions

[PATCH] ipa/96291: don't crash on unoptimized lto functions

[Sergei Trofimovich]

From: Sergei Trofimovich <siarheit@google.com>

In PR ipa/96291 the test contained an SCC with one
unoptimized function. This tricked ipa-cp into NULL dereference.

has_undead_caller_from_outside_scc_p() did not take into account
that unoptimized funtions don't have IPA summary analysis. and
dereferenced NULL pointer causing an ICE.

	PR ipa/96291
	* ipa-cp.c (has_undead_caller_from_outside_scc_p): Consider
	unoptimized callers as undead.
---
 gcc/ipa-cp.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index b0c8f405260..d5082576962 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -5666,9 +5666,15 @@ has_undead_caller_from_outside_scc_p (struct cgraph_node *node,
 	&& cs->caller->call_for_symbol_thunks_and_aliases
 	  (has_undead_caller_from_outside_scc_p, NULL, true))
       return true;
-    else if (!ipa_edge_within_scc (cs)
-	     && !IPA_NODE_REF (cs->caller)->node_dead)
-      return true;
+    else if (!ipa_edge_within_scc (cs))
+      {
+	/* Unoptimized callers don't have IPA information.
+	   Conservatively assume callers are undead.  */
+	if (!IPA_NODE_REF (cs->caller))
+	  return true;
+	if (!IPA_NODE_REF (cs->caller)->node_dead)
+	  return true;
+      }
   return false;
 }
 
-- 
2.27.0

Re: [PATCH] ipa/96291: don't crash on unoptimized lto functions

[Richard Biener]

On Sat, Jul 25, 2020 at 8:35 PM Sergei Trofimovich via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> From: Sergei Trofimovich <siarheit@google.com>
>
> In PR ipa/96291 the test contained an SCC with one
> unoptimized function. This tricked ipa-cp into NULL dereference.
>
> has_undead_caller_from_outside_scc_p() did not take into account
> that unoptimized funtions don't have IPA summary analysis. and
> dereferenced NULL pointer causing an ICE.

Can you create a single-unit testcase with a SCC with one function
having the no_ipa attribute?

>         PR ipa/96291
>         * ipa-cp.c (has_undead_caller_from_outside_scc_p): Consider
>         unoptimized callers as undead.
> ---
>  gcc/ipa-cp.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index b0c8f405260..d5082576962 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -5666,9 +5666,15 @@ has_undead_caller_from_outside_scc_p (struct cgraph_node *node,
>         && cs->caller->call_for_symbol_thunks_and_aliases
>           (has_undead_caller_from_outside_scc_p, NULL, true))
>        return true;
> -    else if (!ipa_edge_within_scc (cs)
> -            && !IPA_NODE_REF (cs->caller)->node_dead)
> -      return true;
> +    else if (!ipa_edge_within_scc (cs))
> +      {
> +       /* Unoptimized callers don't have IPA information.
> +          Conservatively assume callers are undead.  */
> +       if (!IPA_NODE_REF (cs->caller))
> +         return true;
> +       if (!IPA_NODE_REF (cs->caller)->node_dead)
> +         return true;
> +      }
>    return false;
>  }
>
> --
> 2.27.0
>

Re: [PATCH] ipa/96291: don't crash on unoptimized lto functions

[Martin Jambor]

Hi,

On Sat, Jul 25 2020, Sergei Trofimovich wrote:
> From: Sergei Trofimovich <siarheit@google.com>
>
> In PR ipa/96291 the test contained an SCC with one
> unoptimized function. This tricked ipa-cp into NULL dereference.
>
> has_undead_caller_from_outside_scc_p() did not take into account
> that unoptimized funtions don't have IPA summary analysis. and
> dereferenced NULL pointer causing an ICE.
>
> 	PR ipa/96291
> 	* ipa-cp.c (has_undead_caller_from_outside_scc_p): Consider
> 	unoptimized callers as undead.
> ---
>  gcc/ipa-cp.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index b0c8f405260..d5082576962 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -5666,9 +5666,15 @@ has_undead_caller_from_outside_scc_p (struct cgraph_node *node,
>  	&& cs->caller->call_for_symbol_thunks_and_aliases
>  	  (has_undead_caller_from_outside_scc_p, NULL, true))
>        return true;
> -    else if (!ipa_edge_within_scc (cs)
> -	     && !IPA_NODE_REF (cs->caller)->node_dead)
> -      return true;
> +    else if (!ipa_edge_within_scc (cs))
> +      {
> +	/* Unoptimized callers don't have IPA information.
> +	   Conservatively assume callers are undead.  */
> +	if (!IPA_NODE_REF (cs->caller))
> +	  return true;
> +	if (!IPA_NODE_REF (cs->caller)->node_dead)
> +	  return true;

I'd prefer a single condition, i.e.:

    else if (!ipa_edge_within_scc (cs)
	     && (!IPA_NODE_REF (cs->caller)
		 || !IPA_NODE_REF (cs->caller)->node_dead))
      return true;


so OK with that change.

Thanks a lot for looking into this.

Martin

Re: [PATCH] ipa/96291: don't crash on unoptimized lto functions

[Martin Jambor]

Hi,

On Mon, Jul 27 2020, Richard Biener via Gcc-patches wrote:
> On Sat, Jul 25, 2020 at 8:35 PM Sergei Trofimovich via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> From: Sergei Trofimovich <siarheit@google.com>
>>
>> In PR ipa/96291 the test contained an SCC with one
>> unoptimized function. This tricked ipa-cp into NULL dereference.
>>
>> has_undead_caller_from_outside_scc_p() did not take into account
>> that unoptimized funtions don't have IPA summary analysis. and
>> dereferenced NULL pointer causing an ICE.
>
> Can you create a single-unit testcase with a SCC with one function
> having the no_ipa attribute?

This bug is LTO specific because otherwise a summary (although marked as
quite useless) will be left over from the summary building stage.

So Sergei, if you can afford to spend an extra while providing a
testcase, you'll need to add three files into gcc/testsuite/gcc.dg/lto,
with either the second or third (numbered _1 or _2)) having

/* { dg-lto-options { { -flto -O0 } } } */

in them.

Thanks,

Martin



>
>>         PR ipa/96291
>>         * ipa-cp.c (has_undead_caller_from_outside_scc_p): Consider
>>         unoptimized callers as undead.
>> ---
>>  gcc/ipa-cp.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
>> index b0c8f405260..d5082576962 100644
>> --- a/gcc/ipa-cp.c
>> +++ b/gcc/ipa-cp.c
>> @@ -5666,9 +5666,15 @@ has_undead_caller_from_outside_scc_p (struct cgraph_node *node,
>>         && cs->caller->call_for_symbol_thunks_and_aliases
>>           (has_undead_caller_from_outside_scc_p, NULL, true))
>>        return true;
>> -    else if (!ipa_edge_within_scc (cs)
>> -            && !IPA_NODE_REF (cs->caller)->node_dead)
>> -      return true;
>> +    else if (!ipa_edge_within_scc (cs))
>> +      {
>> +       /* Unoptimized callers don't have IPA information.
>> +          Conservatively assume callers are undead.  */
>> +       if (!IPA_NODE_REF (cs->caller))
>> +         return true;
>> +       if (!IPA_NODE_REF (cs->caller)->node_dead)
>> +         return true;
>> +      }
>>    return false;
>>  }
>>
>> --
>> 2.27.0
>>

[PATCH v2] ipa/96291: don't crash on unoptimized lto functions

[Sergei Trofimovich]

From: Sergei Trofimovich <siarheit@google.com>

In PR ipa/96291 the test contained an SCC with one
unoptimized function. This tricked ipa-cp into NULL dereference.

has_undead_caller_from_outside_scc_p() did not take into account
that unoptimized funtions don't have IPA summary analysis. And
dereferenced NULL pointer causing an ICE.

gcc/
	PR ipa/96291
	* ipa-cp.c (has_undead_caller_from_outside_scc_p): Consider
	unoptimized callers as undead.

gcc/testsuite/
	PR ipa/96291
	* gcc.dg/lto/pr96291_0.c: New testcase.
	* gcc.dg/lto/pr96291_1.c: Support file.
	* gcc.dg/lto/pr96291_2.c: Likewise.
	* gcc.dg/lto/pr96291.h: Likewise.
---
 gcc/ipa-cp.c                         |  5 +++--
 gcc/testsuite/gcc.dg/lto/pr96291.h   |  4 ++++
 gcc/testsuite/gcc.dg/lto/pr96291_0.c | 11 +++++++++++
 gcc/testsuite/gcc.dg/lto/pr96291_1.c |  3 +++
 gcc/testsuite/gcc.dg/lto/pr96291_2.c |  7 +++++++
 5 files changed, 28 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/lto/pr96291.h
 create mode 100644 gcc/testsuite/gcc.dg/lto/pr96291_0.c
 create mode 100644 gcc/testsuite/gcc.dg/lto/pr96291_1.c
 create mode 100644 gcc/testsuite/gcc.dg/lto/pr96291_2.c

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index b0c8f405260..fe010ff457c 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -5667,8 +5667,9 @@ has_undead_caller_from_outside_scc_p (struct cgraph_node *node,
 	  (has_undead_caller_from_outside_scc_p, NULL, true))
       return true;
     else if (!ipa_edge_within_scc (cs)
-	     && !IPA_NODE_REF (cs->caller)->node_dead)
-      return true;
+	     && (!IPA_NODE_REF (cs->caller) /* Unoptimized caller.  */
+		 || !IPA_NODE_REF (cs->caller)->node_dead))
+	  return true;
   return false;
 }
 
diff --git a/gcc/testsuite/gcc.dg/lto/pr96291.h b/gcc/testsuite/gcc.dg/lto/pr96291.h
new file mode 100644
index 00000000000..70eb3cb71b8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/lto/pr96291.h
@@ -0,0 +1,4 @@
+void e(void);
+void f(void);
+void a(void *, void *);
+void c(int);
diff --git a/gcc/testsuite/gcc.dg/lto/pr96291_0.c b/gcc/testsuite/gcc.dg/lto/pr96291_0.c
new file mode 100644
index 00000000000..07e63038e03
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/lto/pr96291_0.c
@@ -0,0 +1,11 @@
+/* { dg-lto-do link } */
+
+#include "pr96291.h"
+
+static void * b;
+void c(int d) {
+  f();
+  a(b, b);
+}
+
+void e(void) { c(0); }
diff --git a/gcc/testsuite/gcc.dg/lto/pr96291_1.c b/gcc/testsuite/gcc.dg/lto/pr96291_1.c
new file mode 100644
index 00000000000..44744a94941
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/lto/pr96291_1.c
@@ -0,0 +1,3 @@
+#include "pr96291.h"
+
+void f(void) { c(0); }
diff --git a/gcc/testsuite/gcc.dg/lto/pr96291_2.c b/gcc/testsuite/gcc.dg/lto/pr96291_2.c
new file mode 100644
index 00000000000..5febffbb00c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/lto/pr96291_2.c
@@ -0,0 +1,7 @@
+/* { dg-options {-O0} } */
+
+#include "pr96291.h"
+
+void a(void * a1, void * a2) { e(); }
+
+int main(){}
-- 
2.27.0

Re: [PATCH] ipa/96291: don't crash on unoptimized lto functions

[Sergei Trofimovich]

On Mon, 27 Jul 2020 14:41:14 +0200
Martin Jambor <mjambor@suse.cz> wrote:

> Hi,
> 
> On Mon, Jul 27 2020, Richard Biener via Gcc-patches wrote:
> > On Sat, Jul 25, 2020 at 8:35 PM Sergei Trofimovich via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:  
> >>
> >> From: Sergei Trofimovich <siarheit@google.com>
> >>
> >> In PR ipa/96291 the test contained an SCC with one
> >> unoptimized function. This tricked ipa-cp into NULL dereference.
> >>
> >> has_undead_caller_from_outside_scc_p() did not take into account
> >> that unoptimized funtions don't have IPA summary analysis. and
> >> dereferenced NULL pointer causing an ICE.  
> >
> > Can you create a single-unit testcase with a SCC with one function
> > having the no_ipa attribute?  
> 
> This bug is LTO specific because otherwise a summary (although marked as
> quite useless) will be left over from the summary building stage.

Yeah, I was not able to shrink the example even down to 2 files.

> So Sergei, if you can afford to spend an extra while providing a
> testcase, you'll need to add three files into gcc/testsuite/gcc.dg/lto,
> with either the second or third (numbered _1 or _2)) having

Sounds good! I tried is as:
    https://gcc.gnu.org/pipermail/gcc-patches/2020-July/550817.html

> /* { dg-lto-options { { -flto -O0 } } } */

I used "/* { dg-options {-O0} } */" looking at pr88077_1.c which uses
"/* { dg-options {-fcommon} } */".  But only now looked closer at your
suggestion.

Should I resend with "/* { dg-lto-options { { -flto -O0 } } } */"? I need
a bit or help to understand the difference :)

-- 

  Sergei

Re: [PATCH v2] ipa/96291: don't crash on unoptimized lto functions

[Richard Biener]

On Mon, 27 Jul 2020, Sergei Trofimovich wrote:

> From: Sergei Trofimovich <siarheit@google.com>
> 
> In PR ipa/96291 the test contained an SCC with one
> unoptimized function. This tricked ipa-cp into NULL dereference.
> 
> has_undead_caller_from_outside_scc_p() did not take into account
> that unoptimized funtions don't have IPA summary analysis. And
> dereferenced NULL pointer causing an ICE.

OK.  The dg-options -O0 use in _2.c looks exactly correct.

Thanks,
Richard.

> gcc/
> 	PR ipa/96291
> 	* ipa-cp.c (has_undead_caller_from_outside_scc_p): Consider
> 	unoptimized callers as undead.
> 
> gcc/testsuite/
> 	PR ipa/96291
> 	* gcc.dg/lto/pr96291_0.c: New testcase.
> 	* gcc.dg/lto/pr96291_1.c: Support file.
> 	* gcc.dg/lto/pr96291_2.c: Likewise.
> 	* gcc.dg/lto/pr96291.h: Likewise.
> ---
>  gcc/ipa-cp.c                         |  5 +++--
>  gcc/testsuite/gcc.dg/lto/pr96291.h   |  4 ++++
>  gcc/testsuite/gcc.dg/lto/pr96291_0.c | 11 +++++++++++
>  gcc/testsuite/gcc.dg/lto/pr96291_1.c |  3 +++
>  gcc/testsuite/gcc.dg/lto/pr96291_2.c |  7 +++++++
>  5 files changed, 28 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/lto/pr96291.h
>  create mode 100644 gcc/testsuite/gcc.dg/lto/pr96291_0.c
>  create mode 100644 gcc/testsuite/gcc.dg/lto/pr96291_1.c
>  create mode 100644 gcc/testsuite/gcc.dg/lto/pr96291_2.c
> 
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index b0c8f405260..fe010ff457c 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -5667,8 +5667,9 @@ has_undead_caller_from_outside_scc_p (struct cgraph_node *node,
>  	  (has_undead_caller_from_outside_scc_p, NULL, true))
>        return true;
>      else if (!ipa_edge_within_scc (cs)
> -	     && !IPA_NODE_REF (cs->caller)->node_dead)
> -      return true;
> +	     && (!IPA_NODE_REF (cs->caller) /* Unoptimized caller.  */
> +		 || !IPA_NODE_REF (cs->caller)->node_dead))
> +	  return true;
>    return false;
>  }
>  
> diff --git a/gcc/testsuite/gcc.dg/lto/pr96291.h b/gcc/testsuite/gcc.dg/lto/pr96291.h
> new file mode 100644
> index 00000000000..70eb3cb71b8
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/lto/pr96291.h
> @@ -0,0 +1,4 @@
> +void e(void);
> +void f(void);
> +void a(void *, void *);
> +void c(int);
> diff --git a/gcc/testsuite/gcc.dg/lto/pr96291_0.c b/gcc/testsuite/gcc.dg/lto/pr96291_0.c
> new file mode 100644
> index 00000000000..07e63038e03
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/lto/pr96291_0.c
> @@ -0,0 +1,11 @@
> +/* { dg-lto-do link } */
> +
> +#include "pr96291.h"
> +
> +static void * b;
> +void c(int d) {
> +  f();
> +  a(b, b);
> +}
> +
> +void e(void) { c(0); }
> diff --git a/gcc/testsuite/gcc.dg/lto/pr96291_1.c b/gcc/testsuite/gcc.dg/lto/pr96291_1.c
> new file mode 100644
> index 00000000000..44744a94941
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/lto/pr96291_1.c
> @@ -0,0 +1,3 @@
> +#include "pr96291.h"
> +
> +void f(void) { c(0); }
> diff --git a/gcc/testsuite/gcc.dg/lto/pr96291_2.c b/gcc/testsuite/gcc.dg/lto/pr96291_2.c
> new file mode 100644
> index 00000000000..5febffbb00c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/lto/pr96291_2.c
> @@ -0,0 +1,7 @@
> +/* { dg-options {-O0} } */
> +
> +#include "pr96291.h"
> +
> +void a(void * a1, void * a2) { e(); }
> +
> +int main(){}
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)