diff mbox series

Fix crash with constant initializer

Message ID 2215682.Hq7AAxBmiT@fomalhaut
State New
Headers
Series Fix crash with constant initializer |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Test passed

Commit Message

Eric Botcazou Sept. 30, 2024, 7:55 a.m. UTC 
  Hi,

the attached Ada testcase compiled with -O2 -gnatn makes the compiler crash in 
vect_can_force_dr_alignment_p during SLP vectorization:

  if (decl_in_symtab_p (decl)
      && !symtab_node::get (decl)->can_increase_alignment_p ())
    return false;

because symtab_node::get (decl) returns a null node.  The phenomenon occurs 
for a pair of twin symbols listed like so in .cgraph:

Opt7_Pkg.T12b/17 (Opt7_Pkg.T12b)
  Type: variable definition analyzed
  Visibility: semantic_interposition external public artificial
  Aux: @0x44d45e0
  References: 
  Referring: opt7_pkg__enum_name_table/13 (addr) opt7_pkg__enum_name_table/13 
(addr) 
  Availability: not-ready
  Varpool flags: initialized read-only const-value-known

Opt7_Pkg.T8b/16 (Opt7_Pkg.T8b)
  Type: variable definition analyzed
  Visibility: semantic_interposition external public artificial
  Aux: @0x7f9fda3fff00
  References: 
  Referring: opt7_pkg__enum_name_table/13 (addr) opt7_pkg__enum_name_table/13 
(addr) 
  Availability: not-ready
  Varpool flags: initialized read-only const-value-known

with:

opt7_pkg__enum_name_table/13 (Opt7_Pkg.Enum_Name_Table)
  Type: variable definition analyzed
  Visibility: semantic_interposition external public
  Aux: @0x44d45e0
  References: Opt7_Pkg.T8b/16 (addr) Opt7_Pkg.T8b/16 (addr) Opt7_Pkg.T12b/17 
(addr) Opt7_Pkg.T12b/17 (addr) 
  Referring: opt7_pkg__image/2 (read) opt7_pkg__image/2 (read) 
opt7_pkg__image/2 (read) opt7_pkg__image/2 (read) opt7_pkg__image/2 (read) 
opt7_pkg__image/2 (read) opt7_pkg__image/2 (read) opt7_pkg__image/2 (read) 
  Availability: not-ready
  Varpool flags: initialized read-only const-value-known

being the crux of the matter.

What happens is that symtab_remove_unreachable_nodes leaves the last symbol in 
kind of a limbo state: in .remove_symbols, we have:

opt7_pkg__enum_name_table/13 (Opt7_Pkg.Enum_Name_Table)
  Type: variable
  Body removed by symtab_remove_unreachable_nodes
  Visibility: externally_visible semantic_interposition external public
  References: 
  Referring: opt7_pkg__image/2 (read) opt7_pkg__image/2 (read) 
  Availability: not_available
  Varpool flags: initialized read-only const-value-known

This means that the "body" (DECL_INITIAL) of the symbol has been disregarded 
during reachability analysis, causing the first two symbols to be discarded:

Reclaiming variables: Opt7_Pkg.T12b/17 Opt7_Pkg.T8b/16

but the DECL_INITIAL is explicitly preserved for later constant folding, which 
makes it possible to retrofit the DECLs corresponding to the first two symbols 
in the GIMPLE IR and ultimately leads vect_can_force_dr_alignment_p to crash.


The decision to disregard the "body" (DECL_INITIAL) of the symbol is made in 
the first process_references present in ipa.cc:

      if (node->definition && !node->in_other_partition
	  && ((!DECL_EXTERNAL (node->decl) || node->alias)
	      || (possible_inline_candidate_p (node)
	  /* We use variable constructors during late compilation for
	     constant folding.  Keep references alive so partitioning
	     knows about potential references.  */
		  || (VAR_P (node->decl)
		      && (flag_wpa
			  || flag_incremental_link
			 	 == INCREMENTAL_LINK_LTO)
		      && dyn_cast <varpool_node *> (node)
		      	   ->ctor_useable_for_folding_p ()))))

because neither flag_wpa nor flag_incremental_link = INCREMENTAL_LINK_LTO is 
true, while the decision to ultimately preserve the DECL_INITIAL is made later 
in remove_unreachable_nodes:

  /* Keep body if it may be useful for constant folding.  */
  if ((flag_wpa || flag_incremental_link == INCREMENTAL_LINK_LTO)
      || ((init = ctor_for_folding (vnode->decl)) == error_mark_node))
    vnode->remove_initializer ();
  else
    DECL_INITIAL (vnode->decl) = init;


I think that the testcase shows that the "body" of ctor_useable_for_folding_p 
symbols must always be considered for reachability analysis (which could make 
the above test on ctor_for_folding useless).  But implementing that introduces 
a regression for g++.dg/ipa/devirt-39.C, because the vtable is preserved and 
in turn forces the method to be preserved, hence the special case for vtables.

The test also renames the first process_references function in ipa.cc to clear 
the confusion with the second function in the same file.

Bootstrapped/regtested on x86-64/Linux, OK for the mainline?


2024-09-30  Eric Botcazou  <ebotcazou@adacore.com>

	* ipa.cc (process_references): Rename into...
	(mark_references): ...this.  Always mark referenced external
	variables as reachable if they are usable for folding, except
	for vtables.
	(symbol_table::remove_unreachable_nodes): Adjust to renaming.


2024-09-30  Eric Botcazou  <ebotcazou@adacore.com>

	* gnat.dg/specs/opt7.ads: New test.
	* gnat.dg/specs/opt7_pkg.ads: New helper.
	* gnat.dg/specs/opt7_pkg.adb: Likewise.
diff mbox series

Patch

diff --git a/gcc/ipa.cc b/gcc/ipa.cc
index c453fca5d9b..342fa058160 100644
--- a/gcc/ipa.cc
+++ b/gcc/ipa.cc
@@ -124,41 +124,39 @@  possible_inline_candidate_p (symtab_node *node)
   return lookup_attribute ("always_inline", DECL_ATTRIBUTES (node->decl));
 }
 
-/* Process references.  */
+/* Mark references from SNODE as (members of) REACHABLE and add them to the
+   queue starting at FIRST.  */
 
 static void
-process_references (symtab_node *snode,
-		    symtab_node **first,
-		    hash_set<symtab_node *> *reachable)
+mark_references (symtab_node *snode, symtab_node **first,
+		 hash_set<symtab_node *> *reachable)
 {
-  int i;
   struct ipa_ref *ref = NULL;
-  for (i = 0; snode->iterate_reference (i, ref); i++)
+  for (int i = 0; snode->iterate_reference (i, ref); i++)
     {
       symtab_node *node = ref->referred;
       symtab_node *body = node->ultimate_alias_target ();
 
-      if (node->definition && !node->in_other_partition
-	  && ((!DECL_EXTERNAL (node->decl) || node->alias)
-	      || (possible_inline_candidate_p (node)
-		  /* We use variable constructors during late compilation for
-		     constant folding.  Keep references alive so partitioning
-		     knows about potential references.  */
-		  || (VAR_P (node->decl)
-		      && (flag_wpa
-			  || flag_incremental_link
-			 	 == INCREMENTAL_LINK_LTO)
-		      && dyn_cast <varpool_node *> (node)
-		      	   ->ctor_useable_for_folding_p ()))))
+      if (node->definition
+	  && !node->in_other_partition
+	  && (!DECL_EXTERNAL (node->decl)
+	      || node->alias
+	      || possible_inline_candidate_p (node)
+	      /* We use variable constructors during late compilation for
+		 constant folding, but vtables are handled separately.  */
+	      || (VAR_P (node->decl)
+		  && !DECL_VIRTUAL_P (node->decl)
+		  && dyn_cast <varpool_node *> (node)
+		       ->ctor_useable_for_folding_p ())))
 	{
-	  /* Be sure that we will not optimize out alias target
-	     body.  */
+	  /* Be sure that we will not optimize out alias target body.  */
 	  if (DECL_EXTERNAL (node->decl)
 	      && node->alias
 	      && symtab->state < IPA_SSA_AFTER_INLINING)
 	    reachable->add (body);
 	  reachable->add (node);
 	}
+
       enqueue_node (node, first, reachable);
     }
 }
@@ -409,8 +407,9 @@  symbol_table::remove_unreachable_nodes (FILE *file)
 		    && !reachable.add (next))
 		  enqueue_node (next, &first, &reachable);
 	    }
+
 	  /* Mark references as reachable.  */
-	  process_references (node, &first, &reachable);
+	  mark_references (node, &first, &reachable);
 	}
 
       if (cgraph_node *cnode = dyn_cast <cgraph_node *> (node))