diff mbox

Fix address space computation in expand_debug_expr

Message ID 20140605121929.GA21817@atmel.com
State New
Headers show

Commit Message

Senthil Kumar Selvaraj June 5, 2014, 12:19 p.m. UTC 
On Thu, Jun 05, 2014 at 09:46:25AM +0200, Richard Biener wrote:
> 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;

Modified patch attached. This fixes PR 52472 (the ADDR_EXPR case) as well 
as a couple of ICEs in gcc.target/avr/torture/addr-space-2-x.c (MEM_REF
case). I've also added a new testcase for the PR.

I don't have commit access - could someone apply this for me please?
It'd be great if this was backported to 4.9 and 4.8 branches as well.

Regards
Senthil


gcc/ChangeLog

2014-06-05  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

	PR target/52472
	* cfgexpand.c (expand_debug_expr): Use address space of nested
    TREE_TYPE for ADDR_EXPR and MEM_REF.

gcc/testsuite/ChangeLog

2014-06-05  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>

	PR target/52472
	* gcc.target/avr/pr52472.c: New test.

Comments

Jeff Law June 5, 2014, 6:35 p.m. UTC | #1 
On 06/05/14 06:19, Senthil Kumar Selvaraj wrote:

>
> gcc/ChangeLog
>
> 2014-06-05  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>
> 	PR target/52472
> 	* cfgexpand.c (expand_debug_expr): Use address space of nested
>      TREE_TYPE for ADDR_EXPR and MEM_REF.
>
> gcc/testsuite/ChangeLog
>
> 2014-06-05  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
>
> 	PR target/52472
> 	* gcc.target/avr/pr52472.c: New test.
Thanks.  Installed on your behalf.

Jeff
diff mbox

Patch

diff --git gcc/cfgexpand.c gcc/cfgexpand.c
index 8b0e466..e161cb7 100644
--- gcc/cfgexpand.c
+++ gcc/cfgexpand.c
@@ -3941,10 +3941,7 @@  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)));
-      else
-	as = ADDR_SPACE_GENERIC;
+      as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (exp, 0))));
 
       op0 = convert_debug_memory_address (targetm.addr_space.address_mode (as),
 					  op0, as);
@@ -4467,7 +4464,7 @@  expand_debug_expr (tree exp)
 	  return NULL;
 	}
 
-      as = TYPE_ADDR_SPACE (TREE_TYPE (exp));
+      as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (exp)));
       op0 = convert_debug_memory_address (mode, XEXP (op0, 0), as);
 
       return op0;
diff --git gcc/testsuite/gcc.target/avr/pr52472.c gcc/testsuite/gcc.target/avr/pr52472.c
new file mode 100644
index 0000000..701cfb4
--- /dev/null
+++ gcc/testsuite/gcc.target/avr/pr52472.c
@@ -0,0 +1,16 @@ 
+/* { dg-do compile } */
+/* { dg-options "-Os -g -Wno-pointer-to-int-cast" } */
+
+/* This testcase exposes PR52472. expand_debug_expr mistakenly
+   considers the address space of data to be generic, and
+   asserts that PSImode pointers aren't valid in the generic 
+   address space. */
+
+extern const __memx unsigned data[][10];
+
+unsigned long ice (void)
+{
+  unsigned long addr32;
+
+  return addr32 = ((unsigned long) data);
+}