diff mbox series

[RFC] ipa-visibility: Fix ICE in lto-partition caused by incorrect comdat group solving in ipa-visibility

Message ID 20230327085252.3390790-1-xionghuluo@tencent.com
State Accepted
Headers
Series [RFC] ipa-visibility: Fix ICE in lto-partition caused by incorrect comdat group solving in ipa-visibility |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Xionghu Luo March 27, 2023, 8:52 a.m. UTC 
  I have a case ICE in lto-partion.c:158 not easy to reduce, this ICE
appears some time ago from link:
https://gcc.gnu.org/pipermail/gcc-patches/2021-December/586290.html.
I tried the proposed patch but it doesn't work for me.

Then I did some hack and finally got a successful lto link to compare
with a ICE lto link.  It seems to me that the ICE in
add_symbol_to_partition_1, at lto/lto-partition.c:158
is caused by not dissovle comdat_group_list correctly in
update_visibility_by_resolution_info.

The ICE node is a preempted_reg '__dt_del' function with same_comdat_group
linked to '__dt_base' function, succeeded by '__dt_comp' function.

Success resolution is:

2420 f75f1945 PREVAILING_DEF _ZN6google8protobuf8internal16FunctionClosure1IPKNS0_15FieldDescriptorEED2Ev/81027
2422 f75f1945 PREVAILING_DEF _ZN6google8protobuf8internal16FunctionClosure1IPKNS0_15FieldDescriptorEED1Ev/81028
2424 f75f1945 PREEMPTED_REG _ZN6google8protobuf8internal16FunctionClosure1IPKNS0_15FieldDescriptorEED0Ev/81029

with FOR_EACH_FUNCTION access order:
81029(__dt_del) -> 81028(__dt_comp) -> 81027(__dt_base)

81029 is accessed first, and it is markded externally_visable false,
then accessing 81028 removed all the same_comdat_groups as expected.

ICE resolution is:

2362 f75f1945 PREEMPTED_REG _ZN6google8protobuf8internal16FunctionClosure1IPKNS0_15FieldDescriptorEED0Ev/81029
2365 f75f1945 PREVAILING_DEF _ZN6google8protobuf8internal16FunctionClosure1IPKNS0_15FieldDescriptorEED2Ev/81027
2367 f75f1945 PREVAILING_DEF _ZN6google8protobuf8internal16FunctionClosure1IPKNS0_15FieldDescriptorEED1Ev/81028

with FOR_EACH_FUNCTION access order:
81028(__dt_comp) -> 81027(__dt_base) -> 81029(__dt_del)

81028 is accessed firstly, and node 81029's externally_visible is still
true, when calling 81028's update_visibility_by_resolution_info,
it early returns as 'same_def' is false then fail to dissolve
same_comdat_group list.
So the point here is if PREEMPTED_REG node is not accessed first, the
externallay_visable variable won't be updated on time when accessing
PREVAILING_DEF nodes first.
This patch using *function check* instead of *variable check* to eliminate
the resolution sequence influence.
Not sure whether this patch could also fix Sandra's ICE either?

gcc/ChangeLog:

	* ipa-visibility.cc (update_visibility_by_resolution_info):
	Check node's externally_visiable with function instead of
	variable.
	(function_and_variable_visibility): New parameter whole_program.

Signed-off-by: Xionghu Luo <xionghuluo@tencent.com>
---
 gcc/ipa-visibility.cc | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
diff mbox series

Patch

diff --git a/gcc/ipa-visibility.cc b/gcc/ipa-visibility.cc
index 8ec82bb333e..1ebc584ffd9 100644
--- a/gcc/ipa-visibility.cc
+++ b/gcc/ipa-visibility.cc
@@ -393,7 +393,7 @@  update_vtable_references (tree *tp, int *walk_subtrees,
    resolution info.  */
 
 static void
-update_visibility_by_resolution_info (symtab_node * node)
+update_visibility_by_resolution_info (symtab_node * node, bool whole_program)
 {
   bool define;
 
@@ -412,7 +412,12 @@  update_visibility_by_resolution_info (symtab_node * node)
     for (symtab_node *next = node->same_comdat_group;
 	 next != node; next = next->same_comdat_group)
       {
-	if (!next->externally_visible || next->transparent_alias)
+	if ((is_a<varpool_node *> (next)
+	     && !dyn_cast<varpool_node *> (next)->externally_visible_p ())
+	    || (is_a<cgraph_node *> (next)
+		&& !cgraph_externally_visible_p (dyn_cast<cgraph_node *> (next),
+						 whole_program))
+	    || next->transparent_alias)
 	  continue;
 
 	bool same_def
@@ -750,7 +755,7 @@  function_and_variable_visibility (bool whole_program)
 	    DECL_EXTERNAL (node->decl) = 1;
 	}
 
-      update_visibility_by_resolution_info (node);
+      update_visibility_by_resolution_info (node, whole_program);
       if (node->weakref)
 	optimize_weakref (node);
     }
@@ -842,7 +847,7 @@  function_and_variable_visibility (bool whole_program)
 	  && !DECL_EXTERNAL (vnode->decl))
 	localize_node (whole_program, vnode);
 
-      update_visibility_by_resolution_info (vnode);
+      update_visibility_by_resolution_info (vnode, whole_program);
 
       /* Update virtual tables to point to local aliases where possible.  */
       if (DECL_VIRTUAL_P (vnode->decl)