[patch] Fix debug info for modified parameter

[patch] Fix debug info for modified parameter

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 902 bytes --]

Hi,

since the switch to SSA form at -O0, the compiler generates wrong debug info 
for something like:

void
foo (int i)
{
  int j = 0;
  i = 1;
  j = j + 1;  /* BREAK */
}

If you try to display the value of i after breaking in GDB, you don't get 1.
The reason is that there is no default def SSA_NAME for i in this case, so no 
partitions get the RTL location of the parameter.

Tentative patch attached, it's admittedly a little heavy, but I don't see any 
other solution.  Tested on x86_64-suse-linux, OK for the mainline?


2013-11-11  Eric Botcazou  <ebotcazou@adacore.com>

	* tree-outof-ssa.c (remove_ssa_form): For a parameter without default
	def, pretend that the single partition that contains all the SSA_NAMEs
	for this parameter, if it exists, also contains the default def.


2013-11-11  Eric Botcazou  <ebotcazou@adacore.com>

	* gcc.dg/guality/param-4.c: New test.


-- 
Eric Botcazou

[-- Attachment #2: tree-outof-ssa.diff --]
[-- Type: text/x-patch, Size: 3496 bytes --]

Index: tree-outof-ssa.c
===================================================================
--- tree-outof-ssa.c	(revision 204444)
+++ tree-outof-ssa.c	(working copy)
@@ -972,7 +972,10 @@ expand_phi_nodes (struct ssaexpand *sa)
 static void
 remove_ssa_form (bool perform_ter, struct ssaexpand *sa)
 {
+  tree fndecl = current_function_decl, arg;
   bitmap values = NULL;
+  bitmap parameter_has_default_def;
+  struct pointer_map_t *parameter_map;
   var_map map;
   unsigned i;
 
@@ -999,17 +1002,85 @@ remove_ssa_form (bool perform_ter, struc
 
   sa->map = map;
   sa->values = values;
+
+  /* Find out which partitions contain a default def.  If, for a parameter, no
+     partitions contain a default def for this parameter, then we pretend that
+     the single partition that contains all the SSA_NAMEs for this parameter
+     also contains the (ghost) default def, if such a partition exists.  This
+     makes it possible to have correct debug info, without optimization, for
+     parameters written to before being read.  */
   sa->partition_has_default_def = BITMAP_ALLOC (NULL);
+  parameter_has_default_def = BITMAP_ALLOC (NULL);
+  parameter_map = pointer_map_create ();
+
   for (i = 1; i < num_ssa_names; i++)
     {
-      tree t = ssa_name (i);
-      if (t && SSA_NAME_IS_DEFAULT_DEF (t))
+      tree t, var;
+
+      t = ssa_name (i);
+      if (t == NULL_TREE)
+	continue;
+
+      var = SSA_NAME_VAR (t);
+
+      /* If this is a default def in a partition, set the appropriate flag for
+	 the partition and, if this is a SSA_NAME for a parameter, record that
+	 there is a default def for the parameter.  */
+      if (SSA_NAME_IS_DEFAULT_DEF (t))
+	{
+	  int p = var_to_partition (map, t);
+	  if (p != NO_PARTITION)
+	    {
+	      bitmap_set_bit (sa->partition_has_default_def, p);
+	      if (var && TREE_CODE (var) == PARM_DECL)
+		bitmap_set_bit (parameter_has_default_def, DECL_UID (var));
+	    }
+	}
+
+      /* Otherwise, if this is a SSA_NAME for a parameter without default def,
+	 at least yet, and in a partition, record the partition as associated
+	 with the parameter, unless there is already a different one recorded,
+	 in which case record NO_PARTITION.  */
+      else if (var && TREE_CODE (var) == PARM_DECL
+	       && !bitmap_bit_p (parameter_has_default_def, DECL_UID (var)))
 	{
 	  int p = var_to_partition (map, t);
 	  if (p != NO_PARTITION)
-	    bitmap_set_bit (sa->partition_has_default_def, p);
+	    {
+	      void **slot = pointer_map_contains (parameter_map, var);
+	      if (slot)
+		{
+		  int oldp = (int) (intptr_t) *slot;
+		  if (oldp != p)
+		    *slot = (void *) (intptr_t) NO_PARTITION;
+		}
+	      else
+		{
+		  slot = pointer_map_insert (parameter_map, var);
+		  *slot = (void *) (intptr_t) p;
+		}
+	    }
 	}
     }
+
+  /* Finally walk the scalar parameters without default def and mark that
+     the partition that contains all the SSA_NAMEs for a parameter, if it
+     exists, contains the (ghost) default def.  */
+  for (arg = DECL_ARGUMENTS (fndecl); arg; arg = DECL_CHAIN (arg))
+    if (is_gimple_reg_type (TREE_TYPE (arg))
+	&& !bitmap_bit_p (parameter_has_default_def, DECL_UID (arg)))
+      {
+	void **slot = pointer_map_contains (parameter_map, arg);
+	if (slot)
+	  {
+	    int p = (int) (intptr_t) *slot;
+	    if (p != NO_PARTITION)
+	      bitmap_set_bit (sa->partition_has_default_def, p);
+	  }
+      }
+
+  pointer_map_destroy (parameter_map);
+  BITMAP_FREE (parameter_has_default_def);
 }
 
 

[-- Attachment #3: param-4.c --]
[-- Type: text/x-csrc, Size: 256 bytes --]

/* { dg-do run } */
/* { dg-options "-g" } */
/* { dg-skip-if "" { *-*-* }  { "*" } { "-O0" } } */

void
foo (int i)
{
  int j = 0;
  i = 1;
  j = j + 1;  /* BREAK */
}

int
main (void)
{
  foo (0);
  return 0;
}

/* { dg-final { gdb-test 10 "i" "1" } } */

Re: [patch] Fix debug info for modified parameter

[Richard Biener]

On Mon, Nov 11, 2013 at 11:56 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> since the switch to SSA form at -O0, the compiler generates wrong debug info
> for something like:
>
> void
> foo (int i)
> {
>   int j = 0;
>   i = 1;
>   j = j + 1;  /* BREAK */
> }
>
> If you try to display the value of i after breaking in GDB, you don't get 1.
> The reason is that there is no default def SSA_NAME for i in this case, so no
> partitions get the RTL location of the parameter.
>
> Tentative patch attached, it's admittedly a little heavy, but I don't see any
> other solution.  Tested on x86_64-suse-linux, OK for the mainline?

Hmm, at -O0 we should be able to coalesce all SSA names of a
DECL.  So in theory the following should work:

Index: gcc/cfgexpand.c
===================================================================
--- gcc/cfgexpand.c     (revision 204664)
+++ gcc/cfgexpand.c     (working copy)
@@ -1619,7 +1619,7 @@ expand_used_vars (void)
             we don't do anything here.  But those which don't contain the
             default def (representing a temporary based on the parm/result)
             we need to allocate space just like for normal VAR_DECLs.  */
-         if (!bitmap_bit_p (SA.partition_has_default_def, i))
+         if (optimize && !bitmap_bit_p (SA.partition_has_default_def, i))
            {
              expand_one_var (var, true, true);
              gcc_assert (SA.partition_to_pseudo[i]);

eventually restricting handling to !DECL_IGNORED_P / !DECL_ARTIFICIAL
variables.

Richard.

>
> 2013-11-11  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * tree-outof-ssa.c (remove_ssa_form): For a parameter without default
>         def, pretend that the single partition that contains all the SSA_NAMEs
>         for this parameter, if it exists, also contains the default def.
>
>
> 2013-11-11  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gcc.dg/guality/param-4.c: New test.
>
>
> --
> Eric Botcazou

Re: [patch] Fix debug info for modified parameter

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

> Hmm, at -O0 we should be able to coalesce all SSA names of a
> DECL.  So in theory the following should work:

Yes, the attached patch introduces no regressions in the testsuite.  How 
robust is that though?  Do we need some checking for it?


	* cfgexpand.c (expand_used_vars): Allocate space for partitions based
	on PARM_DECLs or RESULT_DECLs only when optimization is enabled.
	* tree-outof-ssa.c (remove_ssa_form): Record which partitions contain
	a default def only when optimization is enabled.
	(finish_out_of_ssa): Adjust.


-- 
Eric Botcazou

[-- Attachment #2: tree-outof-ssa-2.diff --]
[-- Type: text/x-patch, Size: 2306 bytes --]

Index: cfgexpand.c
===================================================================
--- cfgexpand.c	(revision 204678)
+++ cfgexpand.c	(working copy)
@@ -1610,9 +1610,13 @@ expand_used_vars (void)
 	  replace_ssa_name_symbol (var, (tree) *slot);
 	}
 
+      /* Always allocate space for partitions based on VAR_DECLs.  But for
+	 those based on PARM_DECLs or RESULT_DECLs, there is no need to do
+	 so if optimization is disabled because all the SSA_NAMEs based on
+	 these DECLs should have been coalesced into a single partition.  */
       if (TREE_CODE (SSA_NAME_VAR (var)) == VAR_DECL)
 	expand_one_var (var, true, true);
-      else
+      else if (optimize)
 	{
 	  /* This is a PARM_DECL or RESULT_DECL.  For those partitions that
 	     contain the default def (representing the parm or result itself)
Index: tree-outof-ssa.c
===================================================================
--- tree-outof-ssa.c	(revision 204678)
+++ tree-outof-ssa.c	(working copy)
@@ -999,15 +999,23 @@ remove_ssa_form (bool perform_ter, struc
 
   sa->map = map;
   sa->values = values;
-  sa->partition_has_default_def = BITMAP_ALLOC (NULL);
-  for (i = 1; i < num_ssa_names; i++)
+
+  /* If optimization is enabled, record which partitions contain a default
+     def.  If the underlying object is a PARM_DECL or RESULT_DECL, they'll
+     be assigned the canonical RTL location of the object; the other ones
+     will be assigned a stack temporary.  */
+  if (optimize)
     {
-      tree t = ssa_name (i);
-      if (t && SSA_NAME_IS_DEFAULT_DEF (t))
+      sa->partition_has_default_def = BITMAP_ALLOC (NULL);
+      for (i = 1; i < num_ssa_names; i++)
 	{
-	  int p = var_to_partition (map, t);
-	  if (p != NO_PARTITION)
-	    bitmap_set_bit (sa->partition_has_default_def, p);
+	  tree t = ssa_name (i);
+	  if (t && SSA_NAME_IS_DEFAULT_DEF (t))
+	    {
+	      int p = var_to_partition (map, t);
+	      if (p != NO_PARTITION)
+		bitmap_set_bit (sa->partition_has_default_def, p);
+	    }
 	}
     }
 }
@@ -1183,7 +1191,8 @@ finish_out_of_ssa (struct ssaexpand *sa)
   if (sa->values)
     BITMAP_FREE (sa->values);
   delete_var_map (sa->map);
-  BITMAP_FREE (sa->partition_has_default_def);
+  if (optimize)
+    BITMAP_FREE (sa->partition_has_default_def);
   memset (sa, 0, sizeof *sa);
 }
 

Re: [patch] Fix debug info for modified parameter

[Richard Biener]

On Tue, Nov 12, 2013 at 10:30 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Hmm, at -O0 we should be able to coalesce all SSA names of a
>> DECL.  So in theory the following should work:
>
> Yes, the attached patch introduces no regressions in the testsuite.  How
> robust is that though?  Do we need some checking for it?

I think we rely on it elsewhere, like in coalesce_ssa_name ()
(even though we use MUST_COALESCE_COST - 1 which means we'll
not ICE if we are not able to coalesce).

Changing the coalesce cost to MUST_COALESCE_COST for
PARM_DECLs and RESULT_DECLs should add the required
checking (we'll ICE if we cannot honor that coalescing request).
Note that the code also has a !DECL_IGNORED_P check, so
eventually the expansion code change should restrict itself to
that as well.

But the patch is ok, you can followup with the coalesce cost change
and the !DECL_IGNORED_P checking (if you are not confident enough it
works as-is).

Thanks,
Richard.

>
>         * cfgexpand.c (expand_used_vars): Allocate space for partitions based
>         on PARM_DECLs or RESULT_DECLs only when optimization is enabled.
>         * tree-outof-ssa.c (remove_ssa_form): Record which partitions contain
>         a default def only when optimization is enabled.
>         (finish_out_of_ssa): Adjust.
>
>
> --
> Eric Botcazou

Re: [patch] Fix debug info for modified parameter

[Eric Botcazou]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

> Changing the coalesce cost to MUST_COALESCE_COST for
> PARM_DECLs and RESULT_DECLs should add the required
> checking (we'll ICE if we cannot honor that coalescing request).
> Note that the code also has a !DECL_IGNORED_P check, so
> eventually the expansion code change should restrict itself to
> that as well.
> 
> But the patch is ok, you can followup with the coalesce cost change
> and the !DECL_IGNORED_P checking (if you are not confident enough it
> works as-is).

Even if I were, I strongly believe in consistency and the changes you sketched 
here are the ones I should have come up with, had I known my way around the 
coalescing machinery...  So, thanks again for the tip, patch attached, tested 
with no regressions on x86_64-suse-linux.


2013-11-13  Eric Botcazou  <ebotcazou@adacore.com>

	* cfgexpand.c (expand_used_vars): Allocate space for partitions based
	on PARM_DECLs or RESULT_DECLs only if they are ignored for debug info
	or if optimization is enabled.
	* tree-ssa-coalesce.c (coalesce_ssa_name): If optimization is disabled,
	require that all the names based on a PARM_DECL or a RESULT_DECL that
	isn't ignored for debug info be coalesced.


-- 
Eric Botcazou

[-- Attachment #2: param_dbg.diff --]
[-- Type: text/x-patch, Size: 2425 bytes --]

Index: cfgexpand.c
===================================================================
--- cfgexpand.c	(revision 204678)
+++ cfgexpand.c	(working copy)
@@ -1610,9 +1610,15 @@ expand_used_vars (void)
 	  replace_ssa_name_symbol (var, (tree) *slot);
 	}
 
+      /* Always allocate space for partitions based on VAR_DECLs.  But for
+	 those based on PARM_DECLs or RESULT_DECLs and which matter for the
+	 debug info, there is no need to do so if optimization is disabled
+	 because all the SSA_NAMEs based on these DECLs have been coalesced
+	 into a single partition, which is thus assigned the canonical RTL
+	 location of the DECLs.  */
       if (TREE_CODE (SSA_NAME_VAR (var)) == VAR_DECL)
 	expand_one_var (var, true, true);
-      else
+      else if (DECL_IGNORED_P (SSA_NAME_VAR (var)) || optimize)
 	{
 	  /* This is a PARM_DECL or RESULT_DECL.  For those partitions that
 	     contain the default def (representing the parm or result itself)
Index: tree-ssa-coalesce.c
===================================================================
--- tree-ssa-coalesce.c	(revision 204678)
+++ tree-ssa-coalesce.c	(working copy)
@@ -1256,8 +1256,8 @@ coalesce_ssa_name (void)
   cl = create_coalesce_list ();
   map = create_outofssa_var_map (cl, used_in_copies);
 
-  /* We need to coalesce all names originating same SSA_NAME_VAR
-     so debug info remains undisturbed.  */
+  /* If optimization is disabled, we need to coalesce all the names originating
+     from the same SSA_NAME_VAR so debug info remains undisturbed.  */
   if (!optimize)
     {
       hash_table <ssa_name_var_hash> ssa_name_hash;
@@ -1278,8 +1278,16 @@ coalesce_ssa_name (void)
 		*slot = a;
 	      else
 		{
-		  add_coalesce (cl, SSA_NAME_VERSION (a), SSA_NAME_VERSION (*slot),
-				MUST_COALESCE_COST - 1);
+		  /* If the variable is a PARM_DECL or a RESULT_DECL, we
+		     _require_ that all the names originating from it be
+		     coalesced, because there must be a single partition
+		     containing all the names so that it can be assigned
+		     the canonical RTL location of the DECL safely.  */
+		  const int cost
+		    = TREE_CODE (SSA_NAME_VAR (a)) == VAR_DECL
+		      ? MUST_COALESCE_COST - 1 : MUST_COALESCE_COST;
+		  add_coalesce (cl, SSA_NAME_VERSION (a),
+				SSA_NAME_VERSION (*slot), cost);
 		  bitmap_set_bit (used_in_copies, SSA_NAME_VERSION (a));
 		  bitmap_set_bit (used_in_copies, SSA_NAME_VERSION (*slot));
 		}

Re: [patch] Fix debug info for modified parameter

[Richard Biener]

On Wed, Nov 13, 2013 at 12:33 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Changing the coalesce cost to MUST_COALESCE_COST for
>> PARM_DECLs and RESULT_DECLs should add the required
>> checking (we'll ICE if we cannot honor that coalescing request).
>> Note that the code also has a !DECL_IGNORED_P check, so
>> eventually the expansion code change should restrict itself to
>> that as well.
>>
>> But the patch is ok, you can followup with the coalesce cost change
>> and the !DECL_IGNORED_P checking (if you are not confident enough it
>> works as-is).
>
> Even if I were, I strongly believe in consistency and the changes you sketched
> here are the ones I should have come up with, had I known my way around the
> coalescing machinery...  So, thanks again for the tip, patch attached, tested
> with no regressions on x86_64-suse-linux.

Ok.

Thanks,
Richard.

>
> 2013-11-13  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * cfgexpand.c (expand_used_vars): Allocate space for partitions based
>         on PARM_DECLs or RESULT_DECLs only if they are ignored for debug info
>         or if optimization is enabled.
>         * tree-ssa-coalesce.c (coalesce_ssa_name): If optimization is disabled,
>         require that all the names based on a PARM_DECL or a RESULT_DECL that
>         isn't ignored for debug info be coalesced.
>
>
> --
> Eric Botcazou