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

Message ID 20200725183500.1189534-1-slyfox@gentoo.org
State New
Headers show
Series
  • ipa/96291: don't crash on unoptimized lto functions
Related show

Commit Message

Mike Crowe via Gcc-patches July 25, 2020, 6:35 p.m.
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(-)

-- 
2.27.0

Comments

Mike Crowe via Gcc-patches July 27, 2020, 7:16 a.m. | #1
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

>
Martin Jambor July 27, 2020, 12:36 p.m. | #2
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
Martin Jambor July 27, 2020, 12:41 p.m. | #3
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

>>
Mike Crowe via Gcc-patches July 27, 2020, 9:20 p.m. | #4
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

Patch

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;
 }