Fix reference to freed data in df-scan.c

Fix reference to freed data in df-scan.c

[Richard Sandiford]

While experimenting with some allocation changes I noticed that
df_insn_rescan frees a df_insn_info and implicitly requires
alloc-pool to give back the same data on reallocation:

      bool the_same = df_insn_refs_verify (&collection_rec, bb, insn, false);
      /* If there's no change, return false. */
      if (the_same)
        {
          df_free_collection_rec (&collection_rec);
          if (dump_file)
            fprintf (dump_file, "verify found no changes in insn with uid = %d.\n", uid);
          return false;
        }
      if (dump_file)
        fprintf (dump_file, "rescanning insn with uid = %d.\n", uid);

      /* There's change - we need to delete the existing info.
         Since the insn isn't moved, we can salvage its LUID.  */
      luid = DF_INSN_LUID (insn);
      df_insn_info_delete (uid);
      df_insn_create_insn_record (insn);
      DF_INSN_LUID (insn) = luid;

We build up in collection_rec the list of references that INSN should
have, then exit early if the df info already matches.  Otherwise we
tear down the old df_insn_info, allocate a new one, and copy the
references in collection_rec to it.  The problem is that the references
in collection_rec refer to the old (freed) df_insn_info, so things break
if alloc pool gives back a different address.

The patch avoids the unnecessary free and reallocation.  In principle
it should also be a slight compile-time optimisation, but (as expected)
the difference is far too small to be measurable.

Tested on x86_64-linux-gnu.  OK to install?

Thanks,
Richard

gcc/
	* df-scan.c (df_insn_info_init_fields): New function, split out
	from...
	(df_insn_create_insn_record): ...here.
	(df_insn_info_free_fields): New function, split out from...
	(df_insn_info_delete): ...here.
	(df_insn_rescan): Use the new functions instead of freeing and
	reallocating the df_insn_info.

diff --git a/gcc/df-scan.c b/gcc/df-scan.c
index 93c2eae..259c959 100644
--- a/gcc/df-scan.c
+++ b/gcc/df-scan.c
@@ -809,6 +809,14 @@ df_reg_chain_unlink (df_ref ref)
   df_free_ref (ref);
 }
 
+/* Initialize INSN_INFO to describe INSN.  */
+
+static void
+df_insn_info_init_fields (df_insn_info *insn_info, rtx_insn *insn)
+{
+  memset (insn_info, 0, sizeof (struct df_insn_info));
+  insn_info->insn = insn;
+}
 
 /* Create the insn record for INSN.  If there was one there, zero it
    out.  */
@@ -827,8 +835,7 @@ df_insn_create_insn_record (rtx_insn *insn)
       insn_rec = problem_data->insn_pool->allocate ();
       DF_INSN_INFO_SET (insn, insn_rec);
     }
-  memset (insn_rec, 0, sizeof (struct df_insn_info));
-  insn_rec->insn = insn;
+  df_insn_info_init_fields (insn_rec, insn);
   return insn_rec;
 }
 
@@ -876,6 +883,29 @@ df_mw_hardreg_chain_delete (struct df_mw_hardreg *hardregs)
     }
 }
 
+/* Remove the contents of INSN_INFO (but don't free INSN_INFO itself).  */
+
+static void
+df_insn_info_free_fields (df_insn_info *insn_info)
+{
+  /* In general, notes do not have the insn_info fields
+     initialized.  However, combine deletes insns by changing them
+     to notes.  How clever.  So we cannot just check if it is a
+     valid insn before short circuiting this code, we need to see
+     if we actually initialized it.  */
+  df_mw_hardreg_chain_delete (insn_info->mw_hardregs);
+
+  if (df_chain)
+    {
+      df_ref_chain_delete_du_chain (insn_info->defs);
+      df_ref_chain_delete_du_chain (insn_info->uses);
+      df_ref_chain_delete_du_chain (insn_info->eq_uses);
+    }
+
+  df_ref_chain_delete (insn_info->defs);
+  df_ref_chain_delete (insn_info->uses);
+  df_ref_chain_delete (insn_info->eq_uses);
+}
 
 /* Delete all of the refs information from the insn with UID.
    Internal helper for df_insn_delete, df_insn_rescan, and other
@@ -895,24 +925,7 @@ df_insn_info_delete (unsigned int uid)
       struct df_scan_problem_data *problem_data
 	= (struct df_scan_problem_data *) df_scan->problem_data;
 
-      /* In general, notes do not have the insn_info fields
-	 initialized.  However, combine deletes insns by changing them
-	 to notes.  How clever.  So we cannot just check if it is a
-	 valid insn before short circuiting this code, we need to see
-	 if we actually initialized it.  */
-      df_mw_hardreg_chain_delete (insn_info->mw_hardregs);
-
-      if (df_chain)
-	{
-	  df_ref_chain_delete_du_chain (insn_info->defs);
-	  df_ref_chain_delete_du_chain (insn_info->uses);
-	  df_ref_chain_delete_du_chain (insn_info->eq_uses);
-	}
-
-      df_ref_chain_delete (insn_info->defs);
-      df_ref_chain_delete (insn_info->uses);
-      df_ref_chain_delete (insn_info->eq_uses);
-
+      df_insn_info_free_fields (insn_info);
       problem_data->insn_pool->remove (insn_info);
       DF_INSN_UID_SET (uid, NULL);
     }
@@ -1075,8 +1088,8 @@ df_insn_rescan (rtx_insn *insn)
       /* There's change - we need to delete the existing info.
 	 Since the insn isn't moved, we can salvage its LUID.  */
       luid = DF_INSN_LUID (insn);
-      df_insn_info_delete (uid);
-      df_insn_create_insn_record (insn);
+      df_insn_info_free_fields (insn_info);
+      df_insn_info_init_fields (insn_info, insn);
       DF_INSN_LUID (insn) = luid;
     }
   else

Re: Fix reference to freed data in df-scan.c

[Jeff Law]

On 08/24/2015 05:23 AM, Richard Sandiford wrote:
> While experimenting with some allocation changes I noticed that
> df_insn_rescan frees a df_insn_info and implicitly requires
> alloc-pool to give back the same data on reallocation:
>
>        bool the_same = df_insn_refs_verify (&collection_rec, bb, insn, false);
>        /* If there's no change, return false. */
>        if (the_same)
>          {
>            df_free_collection_rec (&collection_rec);
>            if (dump_file)
>              fprintf (dump_file, "verify found no changes in insn with uid = %d.\n", uid);
>            return false;
>          }
>        if (dump_file)
>          fprintf (dump_file, "rescanning insn with uid = %d.\n", uid);
>
>        /* There's change - we need to delete the existing info.
>           Since the insn isn't moved, we can salvage its LUID.  */
>        luid = DF_INSN_LUID (insn);
>        df_insn_info_delete (uid);
>        df_insn_create_insn_record (insn);
>        DF_INSN_LUID (insn) = luid;
>
> We build up in collection_rec the list of references that INSN should
> have, then exit early if the df info already matches.  Otherwise we
> tear down the old df_insn_info, allocate a new one, and copy the
> references in collection_rec to it.  The problem is that the references
> in collection_rec refer to the old (freed) df_insn_info, so things break
> if alloc pool gives back a different address.
>
> The patch avoids the unnecessary free and reallocation.  In principle
> it should also be a slight compile-time optimisation, but (as expected)
> the difference is far too small to be measurable.
>
> Tested on x86_64-linux-gnu.  OK to install?
>
> Thanks,
> Richard
>
> gcc/
> 	* df-scan.c (df_insn_info_init_fields): New function, split out
> 	from...
> 	(df_insn_create_insn_record): ...here.
> 	(df_insn_info_free_fields): New function, split out from...
> 	(df_insn_info_delete): ...here.
> 	(df_insn_rescan): Use the new functions instead of freeing and
> 	reallocating the df_insn_info.
OK.
jeff
>