On Mon, Jun 18, 2012 at 4:59 PM, Michael Matz <m...@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
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;
}
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;
> +}
