diff mbox

RFA: Fix PR53688

Message ID CAFiYyc0YnBX08Zbdirm2b-hiU0X-0NCp5+K3o-wM9h_S1ekNrQ@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener June 19, 2012, 8:31 a.m. UTC 
On Mon, Jun 18, 2012 at 4:59 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> now that we regard MEM_EXPR as a conservative approximation for MEM_SIZE
> (and MEM_OFFSET) we must ensure that this is really the case.  It isn't
> currently for the string expanders, as they use the MEM_REF (whose address
> was taken) directly as the one to use for MEM_EXPR on the MEM rtx.  That's
> wrong, on gimple side we take the address only and hence its size is
> arbitrary.
>
> So, we have to build a memref always and rewrite its type to one
> representing the real size.  Note that TYPE_MAX_VALUE may be NULL, so we
> don't need to check for 'len' being null or not.
>
> This fixes the C testcase (don't know about fma 3d), and is in
> regstrapping on x86_64-linux.  Okay if that passes?

Ok.

Note that as a followup you should be able to remove the whole

      /* Allow the string and memory builtins to overflow from one
         field into another, see http://gcc.gnu.org/PR23561.
         Thus avoid COMPONENT_REFs in MEM_EXPR unless we know the whole
         memory accessed by the string or memory builtin will fit
         within the field.  */
      if (MEM_EXPR (mem) && TREE_CODE (MEM_EXPR (mem)) == COMPONENT_REF)
        {

block.  Also practically (as we are expanding from GIMPLE now), off
should always be zero and TREE_CODE (exp) should never be
POINTER_PLUS_EXPR, nor should there be wrapping conversions.
The 'off' case can also be dealt with by using the offset operand of
the MEM_REF we build.

Finally MEM_EXPR itself has invalid type-based aliasing properties
(it has so even before your patch), of course that doesn't really matter,
as below we do set_mem_alias_set (mem, 0).  Still with MEM_REF
you should be able to do


note that the NOP stripping was even questionable as it strips conversions
between different address-spaces.

The above should be done, if at all, as a followup to this bugfix of course.

Thanks,
Richard.

>
> Ciao,
> Michael.
>        PR middle-end/53688
>        * builtins.c (get_memory_rtx): Always build a MEM_REF and override
>        its type with one of correct range.
>
> testsuite/
>        * gcc.c-torture/execute/pr53688.c: New test.
> Index: builtins.c
> ===================================================================
> --- builtins.c  (revision 188734)
> +++ builtins.c  (working copy)
> @@ -1274,11 +1274,11 @@ get_memory_rtx (tree exp, tree len)
>       && TREE_CODE (TREE_OPERAND (exp, 0)) == ADDR_EXPR
>       && host_integerp (TREE_OPERAND (exp, 1), 0)
>       && (off = tree_low_cst (TREE_OPERAND (exp, 1), 0)) > 0)
> -    exp = TREE_OPERAND (TREE_OPERAND (exp, 0), 0);
> +    exp = build_simple_mem_ref (TREE_OPERAND (exp, 0));
>   else if (TREE_CODE (exp) == ADDR_EXPR)
> -    exp = TREE_OPERAND (exp, 0);
> +    exp = build_simple_mem_ref (exp);
>   else if (POINTER_TYPE_P (TREE_TYPE (exp)))
> -    exp = build1 (INDIRECT_REF, TREE_TYPE (TREE_TYPE (exp)), exp);
> +    exp = build_simple_mem_ref (exp);
>   else
>     exp = NULL;
>
> @@ -1287,6 +1287,11 @@ get_memory_rtx (tree exp, tree len)
>      (as stringops may access multiple array elements).  */
>   if (exp)
>     {
> +      /* Reset the type to something spanning the whole thing.  EXP
> +         is always a MEM_REF, hence unshared.  */
> +      TREE_TYPE (exp)
> +       = build_array_type (char_type_node,
> +                           build_range_type (sizetype, size_one_node, len));
>       set_mem_attributes (mem, exp, 0);
>
>       if (off)
> Index: testsuite/gcc.c-torture/execute/pr53688.c
> ===================================================================
> --- testsuite/gcc.c-torture/execute/pr53688.c   (revision 0)
> +++ testsuite/gcc.c-torture/execute/pr53688.c   (revision 0)
> @@ -0,0 +1,32 @@
> +char headline[256];
> +struct hdr {
> +  char part1[9];
> +  char part2[8];
> +} p;
> +
> +void __attribute__((noinline,noclone))
> +init()
> +{
> +  __builtin_memcpy (p.part1, "FOOBARFOO", sizeof (p.part1));
> +  __builtin_memcpy (p.part2, "SPEC CPU", sizeof (p.part2));
> +}
> +
> +int main()
> +{
> +  char *x;
> +  int c;
> +  init();
> +  __builtin_memcpy (&headline[0], p.part1, 9);
> +  c = 9;
> +  x = &headline[0];
> +  x = x + c;
> +  __builtin_memset (x, ' ', 245);
> +  __builtin_memcpy (&headline[10], p.part2, 8);
> +  c = 18;
> +  x = &headline[0];
> +  x = x + c;
> +  __builtin_memset (x, ' ', 238);
> +  if (headline[10] != 'S')
> +    __builtin_abort ();
> +  return 0;
> +}

Comments

Michael Matz June 19, 2012, 10:13 a.m. UTC | #1 
Hi,

On Tue, 19 Jun 2012, Richard Guenther wrote:

> > So, we have to build a memref always and rewrite its type to one
> > representing the real size.  Note that TYPE_MAX_VALUE may be NULL, so we
> > don't need to check for 'len' being null or not.
> >
> > This fixes the C testcase (don't know about fma 3d), and is in
> > regstrapping on x86_64-linux.  Okay if that passes?
> 
> Ok.

Thanks, but I now know why we built an INDIRECT_REF :)  
build_simple_mem_ref() only handles some very constrained arguments, 
namely pointers and offseted ADDR_EXPRs when the offset is a constant.  
It doesn't for instance handle &bla->a[i] (it asserts).  So the patch 
trips over the assert in build_simple_mem_ref on "__builtin_memset 
(&p->c[i], 0, 42);".

I could build INDIRECT_REFs always instead of MEM_REFs, this fixes the bug 
too, but it wouldn't generate any MEM_EXPRs anymore, and hence the whole 
bruhaha would be dead code (well, except for alignment setting).

Or I could build MEM_REFs directly, not via build_simple_mem_ref, that 
also works, but leaves us with such MEM_EXPRs sometimes:

  (mem/c:BLK (reg:DI 65) [0 MEM[(void *)&p_1(D)->c[i_2(D)]]+0 A8])

Note the complicated and non-canonical expression in the MEM[].  I'm not 
sure if the disambiguators do anything interesting with such expressions.  
If they aren't we'd safe memory by not generating this MEM_EXPR at all.

If the latter is acceptable, then I indeed can as well wrap everything in 
a MEM_REF like you proposed (possibly with a predicate "simple enough" 
that reflects what build_simple_mem_ref is also checking) and be done with 
it.

So, what should it be?


Ciao,
Michael.
Richard Biener June 19, 2012, 10:55 a.m. UTC | #2 
On Tue, Jun 19, 2012 at 12:13 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Tue, 19 Jun 2012, Richard Guenther wrote:
>
>> > So, we have to build a memref always and rewrite its type to one
>> > representing the real size.  Note that TYPE_MAX_VALUE may be NULL, so we
>> > don't need to check for 'len' being null or not.
>> >
>> > This fixes the C testcase (don't know about fma 3d), and is in
>> > regstrapping on x86_64-linux.  Okay if that passes?
>>
>> Ok.
>
> Thanks, but I now know why we built an INDIRECT_REF :)
> build_simple_mem_ref() only handles some very constrained arguments,
> namely pointers and offseted ADDR_EXPRs when the offset is a constant.
> It doesn't for instance handle &bla->a[i] (it asserts).  So the patch
> trips over the assert in build_simple_mem_ref on "__builtin_memset
> (&p->c[i], 0, 42);".
>
> I could build INDIRECT_REFs always instead of MEM_REFs, this fixes the bug
> too, but it wouldn't generate any MEM_EXPRs anymore, and hence the whole
> bruhaha would be dead code (well, except for alignment setting).
>
> Or I could build MEM_REFs directly, not via build_simple_mem_ref, that
> also works, but leaves us with such MEM_EXPRs sometimes:
>
>  (mem/c:BLK (reg:DI 65) [0 MEM[(void *)&p_1(D)->c[i_2(D)]]+0 A8])
>
> Note the complicated and non-canonical expression in the MEM[].  I'm not
> sure if the disambiguators do anything interesting with such expressions.
> If they aren't we'd safe memory by not generating this MEM_EXPR at all.
>
> If the latter is acceptable, then I indeed can as well wrap everything in
> a MEM_REF like you proposed (possibly with a predicate "simple enough"
> that reflects what build_simple_mem_ref is also checking) and be done with
> it.
>
> So, what should it be?

The MEM_REF is acceptable to the tree oracle and it can extract
points-to information from it.

Thus for simplicity unconditionally building the above is the best.

We can always massage both fold to handle more complex cases
(like the POINTER_PLUS_EXPR case) and set_mem_attributes to
canonicalize / strip the above from useless parts.

Thanks,
Richard.

>
> Ciao,
> Michael.
diff mbox

Patch

Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c      (revision 188733)
+++ gcc/builtins.c      (working copy)
@@ -1250,132 +1250,27 @@  expand_builtin_prefetch (tree exp)
 static rtx
 get_memory_rtx (tree exp, tree len)
 {
-  tree orig_exp = exp;
   rtx addr, mem;
-  HOST_WIDE_INT off;

-  /* When EXP is not resolved SAVE_EXPR, MEM_ATTRS can be still derived
-     from its expression, for expr->a.b only <variable>.a.b is recorded.  */
-  if (TREE_CODE (exp) == SAVE_EXPR && !SAVE_EXPR_RESOLVED_P (exp))
-    exp = TREE_OPERAND (exp, 0);
-
-  addr = expand_expr (orig_exp, NULL_RTX, ptr_mode, EXPAND_NORMAL);
+  addr = expand_expr (exp, NULL_RTX, ptr_mode, EXPAND_NORMAL);
   mem = gen_rtx_MEM (BLKmode, memory_address (BLKmode, addr));

-  /* Get an expression we can use to find the attributes to assign to MEM.
-     If it is an ADDR_EXPR, use the operand.  Otherwise, dereference it if
-     we can.  First remove any nops.  */
-  while (CONVERT_EXPR_P (exp)
-        && POINTER_TYPE_P (TREE_TYPE (TREE_OPERAND (exp, 0))))
-    exp = TREE_OPERAND (exp, 0);
-
-  off = 0;
-  if (TREE_CODE (exp) == POINTER_PLUS_EXPR
-      && TREE_CODE (TREE_OPERAND (exp, 0)) == ADDR_EXPR
-      && host_integerp (TREE_OPERAND (exp, 1), 0)
-      && (off = tree_low_cst (TREE_OPERAND (exp, 1), 0)) > 0)
-    exp = TREE_OPERAND (TREE_OPERAND (exp, 0), 0);
-  else if (TREE_CODE (exp) == ADDR_EXPR)
-    exp = TREE_OPERAND (exp, 0);
-  else if (POINTER_TYPE_P (TREE_TYPE (exp)))
-    exp = build1 (INDIRECT_REF, TREE_TYPE (TREE_TYPE (exp)), exp);
-  else
-    exp = NULL;
+  /* Build a memory reference suitable for MEM_EXPR for use by the
+     alias oracle.  Make sure to give that memory reference a proper
+     access size as well as alias-set zero.  */
+  exp = fold_build2 (MEM_REF,
+                    build_array_type (char_type_node,
+                                      build_range_type (sizetype,
+                                                        size_one_node, len)),
+                    exp, build_int_cst (ptr_type_node, 0));

   /* Honor attributes derived from exp, except for the alias set
      (as builtin stringops may alias with anything) and the size
      (as stringops may access multiple array elements).  */
-  if (exp)
-    {
-      set_mem_attributes (mem, exp, 0);
-
-      if (off)
-       mem = adjust_automodify_address_nv (mem, BLKmode, NULL, off);
-
-      /* Allow the string and memory builtins to overflow from one
-        field into another, see http://gcc.gnu.org/PR23561.
-        Thus avoid COMPONENT_REFs in MEM_EXPR unless we know the whole
-        memory accessed by the string or memory builtin will fit
-        within the field.  */
-      if (MEM_EXPR (mem) && TREE_CODE (MEM_EXPR (mem)) == COMPONENT_REF)
-       {
-         tree mem_expr = MEM_EXPR (mem);
-         HOST_WIDE_INT offset = -1, length = -1;
-         tree inner = exp;
-
-         while (TREE_CODE (inner) == ARRAY_REF
-                || CONVERT_EXPR_P (inner)
-                || TREE_CODE (inner) == VIEW_CONVERT_EXPR
-                || TREE_CODE (inner) == SAVE_EXPR)
-           inner = TREE_OPERAND (inner, 0);
-
-         gcc_assert (TREE_CODE (inner) == COMPONENT_REF);
-
-         if (MEM_OFFSET_KNOWN_P (mem))
-           offset = MEM_OFFSET (mem);
-
-         if (offset >= 0 && len && host_integerp (len, 0))
-           length = tree_low_cst (len, 0);
-
-         while (TREE_CODE (inner) == COMPONENT_REF)
-           {
-             tree field = TREE_OPERAND (inner, 1);
-             gcc_assert (TREE_CODE (mem_expr) == COMPONENT_REF);
-             gcc_assert (field == TREE_OPERAND (mem_expr, 1));
-
-             /* Bitfields are generally not byte-addressable.  */
-             gcc_assert (!DECL_BIT_FIELD (field)
-                         || ((tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1)
-                              % BITS_PER_UNIT) == 0
-                             && host_integerp (DECL_SIZE (field), 0)
-                             && (TREE_INT_CST_LOW (DECL_SIZE (field))
-                                 % BITS_PER_UNIT) == 0));
-
-             /* If we can prove that the memory starting at XEXP (mem, 0) and
-                ending at XEXP (mem, 0) + LENGTH will fit into this field, we
-                can keep the COMPONENT_REF in MEM_EXPR.  But be careful with
-                fields without DECL_SIZE_UNIT like flexible array members.  */
-             if (length >= 0
-                 && DECL_SIZE_UNIT (field)
-                 && host_integerp (DECL_SIZE_UNIT (field), 0))
-               {
-                 HOST_WIDE_INT size
-                   = TREE_INT_CST_LOW (DECL_SIZE_UNIT (field));
-                 if (offset <= size
-                     && length <= size
-                     && offset + length <= size)
-                   break;
-               }
-
-             if (offset >= 0
-                 && host_integerp (DECL_FIELD_OFFSET (field), 0))
-               offset += TREE_INT_CST_LOW (DECL_FIELD_OFFSET (field))
-                         + tree_low_cst (DECL_FIELD_BIT_OFFSET (field), 1)
-                           / BITS_PER_UNIT;
-             else
-               {
-                 offset = -1;
-                 length = -1;
-               }
-
-             mem_expr = TREE_OPERAND (mem_expr, 0);
-             inner = TREE_OPERAND (inner, 0);
-           }
-
-         if (mem_expr == NULL)
-           offset = -1;
-         if (mem_expr != MEM_EXPR (mem))
-           {
-             set_mem_expr (mem, mem_expr);
-             if (offset >= 0)
-               set_mem_offset (mem, offset);
-             else
-               clear_mem_offset (mem);
-           }
-       }
-      set_mem_alias_set (mem, 0);
-      clear_mem_size (mem);
-    }
+  set_mem_attributes (mem, exp, 0);
+  set_mem_alias_set (mem, 0);
+  /* ???  Not sure why we clear mem-size.  */
+  clear_mem_size (mem);

   return mem;
 }