[PATCH] Fix ICE when multiple speculative targets are expanded in different ltrans unit [PR93318]

[PATCH] Fix ICE when multiple speculative targets are expanded in different ltrans unit [PR93318]

[Xionghu Luo]

There is a corner case for speculative multiple targets, that if speculative
edges are streamed to different ltrans units, and then edges are expanded
in one ltrans unit but the speculative property is not cleared by
resolve_speculation in other ltrans unit finally cause ICE.  This patch
fixes this by adding checking to guard speculative edge without no
indirect edge binded to it.  No case is provided since this is from
large program with 128 LTO ltrans units not easy to reduce originated
from protobuf.

Bootstrap and regression tested pass on x86_64-linux-gnu, OK for master?

Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>

gcc/ChangeLog:

	PR ipa/93318
	* cgraph.cc (cgraph_edge::resolve_speculation): Remove
	speculative info if no indirect edge found.
	* cgraph.h: Return NULL if the edges are resloved in other
	ltrans units.
	* tree-inline.cc (copy_bb): Only clone current edge if the
	speculative call is processed in other ltrans units.

Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
---
 gcc/cgraph.cc      |  7 +++++++
 gcc/cgraph.h       |  3 ++-
 gcc/tree-inline.cc | 11 +++++++++++
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
index 7eeda53ca84..120c0ac7650 100644
--- a/gcc/cgraph.cc
+++ b/gcc/cgraph.cc
@@ -1217,6 +1217,13 @@ cgraph_edge::resolve_speculation (cgraph_edge *edge, tree callee_decl)
     e2 = edge;
   ref = e2->speculative_call_target_ref ();
   edge = edge->speculative_call_indirect_edge ();
+  if (!edge)
+  {
+    e2->speculative = false;
+    ref->remove_reference ();
+    return e2;
+  }
+
   if (!callee_decl
       || !ref->referred->semantically_equivalent_p
 	   (symtab_node::get (callee_decl)))
diff --git a/gcc/cgraph.h b/gcc/cgraph.h
index 4be67e3cea9..5404f023e31 100644
--- a/gcc/cgraph.h
+++ b/gcc/cgraph.h
@@ -1773,11 +1773,12 @@ public:
     if (!callee)
       return this;
     for (cgraph_edge *e2 = caller->indirect_calls;
-	 true; e2 = e2->next_callee)
+	 e2; e2 = e2->next_callee)
       if (e2->speculative
 	  && call_stmt == e2->call_stmt
 	  && lto_stmt_uid == e2->lto_stmt_uid)
 	return e2;
+    return NULL;
   }
 
   /* When called on any edge in speculative call and when given any target
diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc
index 043e1d5987a..777d9efdd70 100644
--- a/gcc/tree-inline.cc
+++ b/gcc/tree-inline.cc
@@ -2262,6 +2262,17 @@ copy_bb (copy_body_data *id, basic_block bb,
 
 			  cgraph_edge *indirect
 				 = old_edge->speculative_call_indirect_edge ();
+			  if (indirect == NULL
+			      && old_edge->num_speculative_call_targets_p ()
+				   == 0)
+			    {
+			      cgraph_edge::resolve_speculation (old_edge);
+			      edge = old_edge->clone (id->dst_node, call_stmt,
+						      gimple_uid (stmt), num,
+						      den, true);
+			      edge->count = copy_basic_block->count;
+			      break;
+			    }
 			  profile_count indir_cnt = indirect->count;
 
 			  /* Next iterate all direct edges, clone it and its
-- 
2.27.0

Re: [PATCH] Fix ICE when multiple speculative targets are expanded in different ltrans unit [PR93318]

[Martin Jambor]

Hi,

On Thu, Jun 23 2022, Xionghu Luo via Gcc-patches wrote:
> There is a corner case for speculative multiple targets, that if speculative
> edges are streamed to different ltrans units, and then edges are expanded
> in one ltrans unit but the speculative property is not cleared by
> resolve_speculation in other ltrans unit finally cause ICE.

I am sorry but this does not sound right.  The ltrans compilations are
different processes and changes in the call graph of one cannot alter
the others.

In the ICEing ltrans, at what point does the speculative edge become
lacking an indirect counterpart?  If filing a bug report is impractical,
can you please try figuring that out with a gdb conditional breakpoint
on cgraph_edge::remove if it gets removed and/or watch point on its
speculative flag if its speculation gets resolved?

IMHO that point seems to the be the correct time to avoid this
situation, rather than gracefully ignoring it later.

> This patch fixes this by adding checking to guard speculative edge
> without no indirect edge binded to it.  No case is provided since this
> is from large program with 128 LTO ltrans units not easy to reduce
> originated from protobuf.
>
> Bootstrap and regression tested pass on x86_64-linux-gnu, OK for master?
>
> Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
>
> gcc/ChangeLog:
>
> 	PR ipa/93318
> 	* cgraph.cc (cgraph_edge::resolve_speculation): Remove
> 	speculative info if no indirect edge found.
> 	* cgraph.h: Return NULL if the edges are resloved in other
> 	ltrans units.
> 	* tree-inline.cc (copy_bb): Only clone current edge if the
> 	speculative call is processed in other ltrans units.
>
> Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
> ---
>  gcc/cgraph.cc      |  7 +++++++
>  gcc/cgraph.h       |  3 ++-
>  gcc/tree-inline.cc | 11 +++++++++++
>  3 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc
> index 7eeda53ca84..120c0ac7650 100644
> --- a/gcc/cgraph.cc
> +++ b/gcc/cgraph.cc
> @@ -1217,6 +1217,13 @@ cgraph_edge::resolve_speculation (cgraph_edge *edge, tree callee_decl)
>      e2 = edge;
>    ref = e2->speculative_call_target_ref ();
>    edge = edge->speculative_call_indirect_edge ();
> +  if (!edge)
> +  {
> +    e2->speculative = false;
> +    ref->remove_reference ();
> +    return e2;
> +  }
> +
>    if (!callee_decl
>        || !ref->referred->semantically_equivalent_p
>  	   (symtab_node::get (callee_decl)))
> diff --git a/gcc/cgraph.h b/gcc/cgraph.h
> index 4be67e3cea9..5404f023e31 100644
> --- a/gcc/cgraph.h
> +++ b/gcc/cgraph.h
> @@ -1773,11 +1773,12 @@ public:
>      if (!callee)
>        return this;
>      for (cgraph_edge *e2 = caller->indirect_calls;
> -	 true; e2 = e2->next_callee)
> +	 e2; e2 = e2->next_callee)
>        if (e2->speculative
>  	  && call_stmt == e2->call_stmt
>  	  && lto_stmt_uid == e2->lto_stmt_uid)
>  	return e2;
> +    return NULL;
>    }
>  
>    /* When called on any edge in speculative call and when given any target
> diff --git a/gcc/tree-inline.cc b/gcc/tree-inline.cc
> index 043e1d5987a..777d9efdd70 100644
> --- a/gcc/tree-inline.cc
> +++ b/gcc/tree-inline.cc
> @@ -2262,6 +2262,17 @@ copy_bb (copy_body_data *id, basic_block bb,
>  
>  			  cgraph_edge *indirect
>  				 = old_edge->speculative_call_indirect_edge ();
> +			  if (indirect == NULL
> +			      && old_edge->num_speculative_call_targets_p ()
> +				   == 0)
> +			    {
> +			      cgraph_edge::resolve_speculation (old_edge);
> +			      edge = old_edge->clone (id->dst_node, call_stmt,

using an edge after you have called resolve_speculation on it is bad,
even if you know that the current implementation won't deallocate it
from under you.

But again, I tend to thing the problem must be solved elsewhere.

Thanks,

Martin