diff mbox

Fix address space computation in expand_debug_expr

Message ID 20140604200641.GA10914@atmel.com
State New
Headers show

Commit Message

Senthil Kumar Selvaraj June 4, 2014, 8:06 p.m. UTC
For the AVR target, assertions in convert_debug_memory_address cause a
couple of ICEs (see PR 52472). Jakub suggested returning a NULL rtx,
which works, but on debugging further, I found that expand_debug_expr
appears to incorrectly compute the address space for ADDR_EXPR and
MEM_REFs.

For ADDR_EXPR, TREE_TYPE(exp) is a POINTER_TYPE (always?), but in 
the generic address space, even if the object whose address is taken 
is in a different address space. expand_debug_expr takes 
TYPE_ADDR_SPACE(TREE_TYPE(exp)) and therefore computes the address 
space as generic. convert_debug_memory_address then asserts that the 
mode is a valid pointer mode in the address space and fails.

Similarly, for MEM_REFs, TREE_TYPE(exp) is the type of the
dereferenced value, and therefore checking if it is a POINTER_TYPE
doesn't help for a single pointer dereference. The address space
gets computed as generic even if the pointer points to a different
address space. The address mode for the generic address space is
passed to convert_debug_memory_address, and the assertion that that mode
must match the mode of the rtx then fails.

The below patch attempts to fix this by picking the right TREE_TYPE to
pass to TYPE_ADDR_SPACE for MEM_REF (use type of arg 0) and 
ADDR_EXPR (check for pointer type and look at nested addr space). 

Does this look reasonable or did I get it all wrong?

Regards
Senthil

Comments

Richard Biener June 5, 2014, 7:46 a.m. UTC | #1
On Wed, Jun 4, 2014 at 10:06 PM, Senthil Kumar Selvaraj
<senthil_kumar.selvaraj@atmel.com> wrote:
> For the AVR target, assertions in convert_debug_memory_address cause a
> couple of ICEs (see PR 52472). Jakub suggested returning a NULL rtx,
> which works, but on debugging further, I found that expand_debug_expr
> appears to incorrectly compute the address space for ADDR_EXPR and
> MEM_REFs.
>
> For ADDR_EXPR, TREE_TYPE(exp) is a POINTER_TYPE (always?), but in
> the generic address space, even if the object whose address is taken
> is in a different address space. expand_debug_expr takes
> TYPE_ADDR_SPACE(TREE_TYPE(exp)) and therefore computes the address
> space as generic. convert_debug_memory_address then asserts that the
> mode is a valid pointer mode in the address space and fails.
>
> Similarly, for MEM_REFs, TREE_TYPE(exp) is the type of the
> dereferenced value, and therefore checking if it is a POINTER_TYPE
> doesn't help for a single pointer dereference. The address space
> gets computed as generic even if the pointer points to a different
> address space. The address mode for the generic address space is
> passed to convert_debug_memory_address, and the assertion that that mode
> must match the mode of the rtx then fails.
>
> The below patch attempts to fix this by picking the right TREE_TYPE to
> pass to TYPE_ADDR_SPACE for MEM_REF (use type of arg 0) and
> ADDR_EXPR (check for pointer type and look at nested addr space).
>
> Does this look reasonable or did I get it all wrong?
>
> Regards
> Senthil
>
> diff --git gcc/cfgexpand.c gcc/cfgexpand.c
> index 8b0e466..ca78953 100644
> --- gcc/cfgexpand.c
> +++ gcc/cfgexpand.c
> @@ -3941,8 +3941,8 @@ expand_debug_expr (tree exp)
>           op0 = plus_constant (inner_mode, op0, INTVAL (op1));
>         }
>
> -      if (POINTER_TYPE_P (TREE_TYPE (exp)))
> -       as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (exp)));
> +      if (POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (exp, 0))))
> +       as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (exp, 0))));
>        else
>         as = ADDR_SPACE_GENERIC;

TREE_OPERAND (exp, 0) always has pointer type so I'd change this to

    as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (exp, 0))));

> @@ -4467,7 +4467,11 @@ expand_debug_expr (tree exp)
>           return NULL;
>         }
>
> -      as = TYPE_ADDR_SPACE (TREE_TYPE (exp));
> +      if (POINTER_TYPE_P (TREE_TYPE (exp)))
> +        as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (exp)));
> +      else
> +        as = TYPE_ADDR_SPACE (TREE_TYPE (exp));
> +

Likewise - TREE_TYPE (exp) is always a pointer type.  Otherwise the
patch looks ok to me.

Richard.

>        op0 = convert_debug_memory_address (mode, XEXP (op0, 0), as);
>
>        return op0;
diff mbox

Patch

diff --git gcc/cfgexpand.c gcc/cfgexpand.c
index 8b0e466..ca78953 100644
--- gcc/cfgexpand.c
+++ gcc/cfgexpand.c
@@ -3941,8 +3941,8 @@  expand_debug_expr (tree exp)
 	  op0 = plus_constant (inner_mode, op0, INTVAL (op1));
 	}
 
-      if (POINTER_TYPE_P (TREE_TYPE (exp)))
-	as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (exp)));
+      if (POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (exp, 0))))
+	as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (exp, 0))));
       else
 	as = ADDR_SPACE_GENERIC;
 
@@ -4467,7 +4467,11 @@  expand_debug_expr (tree exp)
 	  return NULL;
 	}
 
-      as = TYPE_ADDR_SPACE (TREE_TYPE (exp));
+      if (POINTER_TYPE_P (TREE_TYPE (exp)))
+        as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (exp)));
+      else
+        as = TYPE_ADDR_SPACE (TREE_TYPE (exp));
+
       op0 = convert_debug_memory_address (mode, XEXP (op0, 0), as);
 
       return op0;