[PR41276, PR41307] don't leak VALUEs into VAR_LOCATION NOTEs

[PR41276, PR41307] don't leak VALUEs into VAR_LOCATION NOTEs

[Alexandre Oliva]

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

When a VALUE in var-tracking expands to a hard reg, and we want a subreg
of that value but the hard reg can't hold that mode, we end up returning
the original subreg of a value, which is inappropriate.

This patch ensures that we don't.  I'm going to check it in as obvious.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-cselib-subreg-of-value-pr41276.patch --]
[-- Type: text/x-diff, Size: 1025 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/41276
	PR debug/41307
	* cselib.c (cselib_expand_value_rtx_1): Don't return copy of
	invalid subreg.

Index: gcc/cselib.c
===================================================================
--- gcc/cselib.c.orig	2009-09-07 06:49:48.000000000 -0300
+++ gcc/cselib.c	2009-09-07 06:50:06.000000000 -0300
@@ -1165,12 +1165,12 @@ cselib_expand_value_rtx_1 (rtx orig, str
 	scopy = simplify_gen_subreg (GET_MODE (orig), subreg,
 				     GET_MODE (SUBREG_REG (orig)),
 				     SUBREG_BYTE (orig));
-	if (scopy == NULL
-	    || (GET_CODE (scopy) == SUBREG
-		&& !REG_P (SUBREG_REG (scopy))
-		&& !MEM_P (SUBREG_REG (scopy))
-		&& (REG_P (SUBREG_REG (orig))
-		    || MEM_P (SUBREG_REG (orig)))))
+	if ((scopy == NULL
+	     || (GET_CODE (scopy) == SUBREG
+		 && !REG_P (SUBREG_REG (scopy))
+		 && !MEM_P (SUBREG_REG (scopy))))
+	    && (REG_P (SUBREG_REG (orig))
+		|| MEM_P (SUBREG_REG (orig))))
 	  return shallow_copy_rtx (orig);
 	return scopy;
       }

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: [PR41276, PR41307] don't leak VALUEs into VAR_LOCATION NOTEs

[Jakub Jelinek]

On Tue, Sep 08, 2009 at 12:40:11PM -0300, Alexandre Oliva wrote:
> When a VALUE in var-tracking expands to a hard reg, and we want a subreg
> of that value but the hard reg can't hold that mode, we end up returning
> the original subreg of a value, which is inappropriate.
> 
> This patch ensures that we don't.  I'm going to check it in as obvious.
> 

> for  gcc/ChangeLog
> from  Alexandre Oliva  <aoliva@redhat.com>
> 
> 	PR debug/41276
> 	PR debug/41307
> 	* cselib.c (cselib_expand_value_rtx_1): Don't return copy of
> 	invalid subreg.
> 
> Index: gcc/cselib.c
> ===================================================================
> --- gcc/cselib.c.orig	2009-09-07 06:49:48.000000000 -0300
> +++ gcc/cselib.c	2009-09-07 06:50:06.000000000 -0300
> @@ -1165,12 +1165,12 @@ cselib_expand_value_rtx_1 (rtx orig, str
>  	scopy = simplify_gen_subreg (GET_MODE (orig), subreg,
>  				     GET_MODE (SUBREG_REG (orig)),
>  				     SUBREG_BYTE (orig));
> -	if (scopy == NULL
> -	    || (GET_CODE (scopy) == SUBREG
> -		&& !REG_P (SUBREG_REG (scopy))
> -		&& !MEM_P (SUBREG_REG (scopy))
> -		&& (REG_P (SUBREG_REG (orig))
> -		    || MEM_P (SUBREG_REG (orig)))))
> +	if ((scopy == NULL
> +	     || (GET_CODE (scopy) == SUBREG
> +		 && !REG_P (SUBREG_REG (scopy))
> +		 && !MEM_P (SUBREG_REG (scopy))))
> +	    && (REG_P (SUBREG_REG (orig))
> +		|| MEM_P (SUBREG_REG (orig))))
>  	  return shallow_copy_rtx (orig);
>  	return scopy;
>        }

I think this still isn't right, whenever we shallow_copy_rtx (orig),
its SUBREG_REG will still be invalidly shared (think of MEM with some fancy
address inside it, could even have VALUE in its address and leak to
dwarf2out).

The important question is, do we want less pedantic rules for SUBREGs
in DEBUG_INSNs?  I believe so, already now VTA creates all kinds of stuff in
SUBREG and dwarf2out.c is able to deal with it (there is no
validation performed on DEBUG_INSN argument).  So, for evd->callback != NULL we
could just gen_rtx_SUBREG if simplify_gen_subreg failed, dwarf2out will
still handle it as low part of some register.  And otherwise (for DSE
purposes) we should just return NULL, otherwise we might end up with invalid
RTL sharing.

	Jakub

Re: [PR41276, PR41307] don't leak VALUEs into VAR_LOCATION NOTEs

[Alexandre Oliva]

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

On Sep  8, 2009, Jakub Jelinek <jakub@redhat.com> wrote:

> I think this still isn't right, whenever we shallow_copy_rtx (orig),
> its SUBREG_REG will still be invalidly shared (think of MEM with some fancy
> address inside it, could even have VALUE in its address and leak to
> dwarf2out).

Agreed.  Taking orig without VALUE substitution is never right.

> The important question is, do we want less pedantic rules for SUBREGs
> in DEBUG_INSNs?

No reason not to.

Here's what I'm testing.  Ok to install if it passes regtesting?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-cselib-subreg-of-value-more-pr41276.patch --]
[-- Type: text/x-diff, Size: 4837 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* cselib.c (cselib_expand_value_rtx_cb): Document callback
	interface.
	(cselib_expand_value_rtx_1): Use callback for SUBREGs.  Adjust
	for VALUEs, to implement the documented interface.
	* var-tracking.c (vt_expand_loc_callback): Handle SUBREGs.
	Adjust for VALUEs and anything else, to implement the
	documented interface.

Index: gcc/cselib.c
===================================================================
--- gcc/cselib.c.orig	2009-09-08 19:34:10.000000000 -0300
+++ gcc/cselib.c	2009-09-08 19:35:32.000000000 -0300
@@ -1053,7 +1053,10 @@ cselib_expand_value_rtx (rtx orig, bitma
 }
 
 /* Same as cselib_expand_value_rtx, but using a callback to try to
-   resolve VALUEs that expand to nothing.  */
+   resolve some expressions.  The CB function should return ORIG if it
+   can't or does not want to deal with a certain RTX.  Any other
+   return value, including NULL, will be taken as the expansion for
+   VALUE.  */
 
 rtx
 cselib_expand_value_rtx_cb (rtx orig, bitmap regs_active, int max_depth,
@@ -1068,6 +1071,9 @@ cselib_expand_value_rtx_cb (rtx orig, bi
   return cselib_expand_value_rtx_1 (orig, &evd, max_depth);
 }
 
+/* Internal implementation of cselib_expand_value_rtx and
+   cselib_expand_value_rtx_cb.  */
+
 static rtx
 cselib_expand_value_rtx_1 (rtx orig, struct expand_value_data *evd,
 			   int max_depth)
@@ -1158,20 +1164,29 @@ cselib_expand_value_rtx_1 (rtx orig, str
 
     case SUBREG:
       {
-	rtx subreg = cselib_expand_value_rtx_1 (SUBREG_REG (orig), evd,
-						max_depth - 1);
+	rtx subreg;
+
+	if (evd->callback)
+	  {
+	    subreg = evd->callback (orig, evd->regs_active, max_depth,
+				    evd->callback_arg);
+	    if (subreg != orig)
+	      return subreg;
+	  }
+
+	subreg = cselib_expand_value_rtx_1 (SUBREG_REG (orig), evd,
+					    max_depth - 1);
 	if (!subreg)
 	  return NULL;
 	scopy = simplify_gen_subreg (GET_MODE (orig), subreg,
 				     GET_MODE (SUBREG_REG (orig)),
 				     SUBREG_BYTE (orig));
-	if ((scopy == NULL
-	     || (GET_CODE (scopy) == SUBREG
-		 && !REG_P (SUBREG_REG (scopy))
-		 && !MEM_P (SUBREG_REG (scopy))))
-	    && (REG_P (SUBREG_REG (orig))
-		|| MEM_P (SUBREG_REG (orig))))
-	  return shallow_copy_rtx (orig);
+	if (scopy == NULL
+	    || (GET_CODE (scopy) == SUBREG
+		&& !REG_P (SUBREG_REG (scopy))
+		&& !MEM_P (SUBREG_REG (scopy))))
+	  return NULL;
+
 	return scopy;
       }
 
@@ -1194,7 +1209,7 @@ cselib_expand_value_rtx_1 (rtx orig, str
 	    if (result == orig)
 	      result = NULL;
 	    else if (result)
-	      result = cselib_expand_value_rtx_1 (result, evd, max_depth);
+	      return cselib_expand_value_rtx_1 (result, evd, max_depth);
 	  }
 
 	if (!result)
Index: gcc/var-tracking.c
===================================================================
--- gcc/var-tracking.c.orig	2009-09-08 19:33:55.000000000 -0300
+++ gcc/var-tracking.c	2009-09-08 19:34:35.000000000 -0300
@@ -6243,7 +6243,8 @@ check_wrap_constant (enum machine_mode m
 }
 
 /* Callback for cselib_expand_value, that looks for expressions
-   holding the value in the var-tracking hash tables.  */
+   holding the value in the var-tracking hash tables.  Return X for
+   standard processing, anything else is to be used as-is.  */
 
 static rtx
 vt_expand_loc_callback (rtx x, bitmap regs, int max_depth, void *data)
@@ -6254,19 +6255,46 @@ vt_expand_loc_callback (rtx x, bitmap re
   location_chain loc;
   rtx result;
 
-  gcc_assert (GET_CODE (x) == VALUE);
+  if (GET_CODE (x) == SUBREG)
+    {
+      rtx subreg = SUBREG_REG (x);
+
+      if (GET_CODE (SUBREG_REG (x)) != VALUE)
+	return x;
+
+      subreg = cselib_expand_value_rtx_cb (SUBREG_REG (x), regs,
+					   max_depth - 1,
+					   vt_expand_loc_callback, data);
+
+      if (!subreg)
+	return NULL;
+
+      result = simplify_gen_subreg (GET_MODE (x), subreg,
+				    GET_MODE (SUBREG_REG (x)),
+				    SUBREG_BYTE (x));
+
+      /* Invalid SUBREGs are ok in debug info.  ??? We could try
+	 alternate expansions for the VALUE as well.  */
+      if (!result && (REG_P (subreg) || MEM_P (subreg)))
+	result = gen_rtx_SUBREG (GET_MODE (x), subreg, SUBREG_BYTE (x));
+
+      return result;
+    }
+
+  if (GET_CODE (x) != VALUE)
+    return x;
 
   if (VALUE_RECURSED_INTO (x))
-    return NULL;
+    return x;
 
   dv = dv_from_value (x);
   var = (variable) htab_find_with_hash (vars, dv, dv_htab_hash (dv));
 
   if (!var)
-    return NULL;
+    return x;
 
   if (var->n_var_parts == 0)
-    return NULL;
+    return x;
 
   gcc_assert (var->n_var_parts == 1);
 
@@ -6283,7 +6311,10 @@ vt_expand_loc_callback (rtx x, bitmap re
     }
 
   VALUE_RECURSED_INTO (x) = false;
-  return result;
+  if (result)
+    return result;
+  else
+    return x;
 }
 
 /* Expand VALUEs in LOC, using VARS as well as cselib's equivalence

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: [PR41276, PR41307] don't leak VALUEs into VAR_LOCATION NOTEs

[Jakub Jelinek]

On Tue, Sep 08, 2009 at 07:40:04PM -0300, Alexandre Oliva wrote:
> Here's what I'm testing.  Ok to install if it passes regtesting?
> 

> for  gcc/ChangeLog
> from  Alexandre Oliva  <aoliva@redhat.com>
> 
> 	* cselib.c (cselib_expand_value_rtx_cb): Document callback
> 	interface.
> 	(cselib_expand_value_rtx_1): Use callback for SUBREGs.  Adjust
> 	for VALUEs, to implement the documented interface.
> 	* var-tracking.c (vt_expand_loc_callback): Handle SUBREGs.
> 	Adjust for VALUEs and anything else, to implement the
> 	documented interface.

Looks good to me, that said, I'm not a RTL reviewer, you know ;)

	Jakub

Re: [PR41276, PR41307] don't leak VALUEs into VAR_LOCATION NOTEs

[Eric Botcazou]

> Here's what I'm testing.  Ok to install if it passes regtesting?

I think it must be specified whether the return value of the callback is 
expanded again or not.  With this latest patch it would be for VALUEs but 
wouldn't for SUBREGs; any possibility to unify that either way?

-- 
Eric Botcazou

Re: [PR41276, PR41307] don't leak VALUEs into VAR_LOCATION NOTEs

[Jakub Jelinek]

On Wed, Sep 09, 2009 at 09:46:07AM +0200, Eric Botcazou wrote:
> > Here's what I'm testing.  Ok to install if it passes regtesting?
> 
> I think it must be specified whether the return value of the callback is 
> expanded again or not.  With this latest patch it would be for VALUEs but 
> wouldn't for SUBREGs; any possibility to unify that either way?

To me it looks like for VALUEs we expand them twice:

1) the only way how vt_expand_loc_callback returns non-NULL is through:
      result = cselib_expand_value_rtx_cb (loc->loc, regs, max_depth,
                                           vt_expand_loc_callback, vars);
      result = check_wrap_constant (GET_MODE (loc->loc), result);
      if (result)
        break;

and then

2)          result = evd->callback (orig, evd->regs_active, max_depth,
                                    evd->callback_arg);
            if (result == orig)
              result = NULL;
            else if (result)
              result = cselib_expand_value_rtx_1 (result, evd, max_depth);

I wonder what the cselib_expand_value_rtx_1 call is for.  If we dropped
this, the return value from the callback wouldn't be expanded again in any
case.

	Jakub

Re: [PR41276, PR41307] don't leak VALUEs into VAR_LOCATION NOTEs

[Alexandre Oliva]

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

On Sep  9, 2009, Eric Botcazou <ebotcazou@adacore.com> wrote:

>> Here's what I'm testing.  Ok to install if it passes regtesting?
> I think it must be specified whether the return value of the callback is 
> expanded again or not.  With this latest patch it would be for VALUEs but 
> wouldn't for SUBREGs; any possibility to unify that either way?

Thanks, good catch, I'd failed to adjust the VALUE case to the API
refinement.

Here's the revised patch I'm testing.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-cselib-subreg-of-value-more-pr41276.patch --]
[-- Type: text/x-diff, Size: 5417 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/41276
	PR debug/41307
	* cselib.c (cselib_expand_value_rtx_cb): Document callback
	interface.
	(cselib_expand_value_rtx_1): Use callback for SUBREGs.  Adjust
	for VALUEs, to implement the documented interface.
	* var-tracking.c (vt_expand_loc_callback): Handle SUBREGs.
	Adjust for VALUEs and anything else, to implement the
	documented interface.

Index: gcc/cselib.c
===================================================================
--- gcc/cselib.c.orig	2009-09-08 19:34:10.000000000 -0300
+++ gcc/cselib.c	2009-09-09 05:20:52.000000000 -0300
@@ -1053,7 +1053,10 @@ cselib_expand_value_rtx (rtx orig, bitma
 }
 
 /* Same as cselib_expand_value_rtx, but using a callback to try to
-   resolve VALUEs that expand to nothing.  */
+   resolve some expressions.  The CB function should return ORIG if it
+   can't or does not want to deal with a certain RTX.  Any other
+   return value, including NULL, will be used as the expansion for
+   VALUE, without any further changes.  */
 
 rtx
 cselib_expand_value_rtx_cb (rtx orig, bitmap regs_active, int max_depth,
@@ -1068,6 +1071,9 @@ cselib_expand_value_rtx_cb (rtx orig, bi
   return cselib_expand_value_rtx_1 (orig, &evd, max_depth);
 }
 
+/* Internal implementation of cselib_expand_value_rtx and
+   cselib_expand_value_rtx_cb.  */
+
 static rtx
 cselib_expand_value_rtx_1 (rtx orig, struct expand_value_data *evd,
 			   int max_depth)
@@ -1158,26 +1164,36 @@ cselib_expand_value_rtx_1 (rtx orig, str
 
     case SUBREG:
       {
-	rtx subreg = cselib_expand_value_rtx_1 (SUBREG_REG (orig), evd,
-						max_depth - 1);
+	rtx subreg;
+
+	if (evd->callback)
+	  {
+	    subreg = evd->callback (orig, evd->regs_active, max_depth,
+				    evd->callback_arg);
+	    if (subreg != orig)
+	      return subreg;
+	  }
+
+	subreg = cselib_expand_value_rtx_1 (SUBREG_REG (orig), evd,
+					    max_depth - 1);
 	if (!subreg)
 	  return NULL;
 	scopy = simplify_gen_subreg (GET_MODE (orig), subreg,
 				     GET_MODE (SUBREG_REG (orig)),
 				     SUBREG_BYTE (orig));
-	if ((scopy == NULL
-	     || (GET_CODE (scopy) == SUBREG
-		 && !REG_P (SUBREG_REG (scopy))
-		 && !MEM_P (SUBREG_REG (scopy))))
-	    && (REG_P (SUBREG_REG (orig))
-		|| MEM_P (SUBREG_REG (orig))))
-	  return shallow_copy_rtx (orig);
+	if (scopy == NULL
+	    || (GET_CODE (scopy) == SUBREG
+		&& !REG_P (SUBREG_REG (scopy))
+		&& !MEM_P (SUBREG_REG (scopy))))
+	  return NULL;
+
 	return scopy;
       }
 
     case VALUE:
       {
 	rtx result;
+
 	if (dump_file && (dump_flags & TDF_DETAILS))
 	  {
 	    fputs ("\nexpanding ", dump_file);
@@ -1185,20 +1201,16 @@ cselib_expand_value_rtx_1 (rtx orig, str
 	    fputs (" into...", dump_file);
 	  }
 
-	if (!evd->callback)
-	  result = NULL;
-	else
+	if (evd->callback)
 	  {
 	    result = evd->callback (orig, evd->regs_active, max_depth,
 				    evd->callback_arg);
-	    if (result == orig)
-	      result = NULL;
-	    else if (result)
-	      result = cselib_expand_value_rtx_1 (result, evd, max_depth);
+
+	    if (result != orig)
+	      return result;
 	  }
 
-	if (!result)
-	  result = expand_loc (CSELIB_VAL_PTR (orig)->locs, evd, max_depth);
+	result = expand_loc (CSELIB_VAL_PTR (orig)->locs, evd, max_depth);
 	return result;
       }
     default:
Index: gcc/var-tracking.c
===================================================================
--- gcc/var-tracking.c.orig	2009-09-08 19:33:55.000000000 -0300
+++ gcc/var-tracking.c	2009-09-08 20:08:08.000000000 -0300
@@ -6243,7 +6243,8 @@ check_wrap_constant (enum machine_mode m
 }
 
 /* Callback for cselib_expand_value, that looks for expressions
-   holding the value in the var-tracking hash tables.  */
+   holding the value in the var-tracking hash tables.  Return X for
+   standard processing, anything else is to be used as-is.  */
 
 static rtx
 vt_expand_loc_callback (rtx x, bitmap regs, int max_depth, void *data)
@@ -6254,19 +6255,46 @@ vt_expand_loc_callback (rtx x, bitmap re
   location_chain loc;
   rtx result;
 
-  gcc_assert (GET_CODE (x) == VALUE);
+  if (GET_CODE (x) == SUBREG)
+    {
+      rtx subreg = SUBREG_REG (x);
+
+      if (GET_CODE (SUBREG_REG (x)) != VALUE)
+	return x;
+
+      subreg = cselib_expand_value_rtx_cb (SUBREG_REG (x), regs,
+					   max_depth - 1,
+					   vt_expand_loc_callback, data);
+
+      if (!subreg)
+	return NULL;
+
+      result = simplify_gen_subreg (GET_MODE (x), subreg,
+				    GET_MODE (SUBREG_REG (x)),
+				    SUBREG_BYTE (x));
+
+      /* Invalid SUBREGs are ok in debug info.  ??? We could try
+	 alternate expansions for the VALUE as well.  */
+      if (!result && (REG_P (subreg) || MEM_P (subreg)))
+	result = gen_rtx_raw_SUBREG (GET_MODE (x), subreg, SUBREG_BYTE (x));
+
+      return result;
+    }
+
+  if (GET_CODE (x) != VALUE)
+    return x;
 
   if (VALUE_RECURSED_INTO (x))
-    return NULL;
+    return x;
 
   dv = dv_from_value (x);
   var = (variable) htab_find_with_hash (vars, dv, dv_htab_hash (dv));
 
   if (!var)
-    return NULL;
+    return x;
 
   if (var->n_var_parts == 0)
-    return NULL;
+    return x;
 
   gcc_assert (var->n_var_parts == 1);
 
@@ -6283,7 +6311,10 @@ vt_expand_loc_callback (rtx x, bitmap re
     }
 
   VALUE_RECURSED_INTO (x) = false;
-  return result;
+  if (result)
+    return result;
+  else
+    return x;
 }
 
 /* Expand VALUEs in LOC, using VARS as well as cselib's equivalence

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: [PR41276, PR41307] don't leak VALUEs into VAR_LOCATION NOTEs

[Eric Botcazou]

> Here's the revised patch I'm testing.

Looks fine to me, thanks.

-- 
Eric Botcazou