On Tue, 3 Dec 2013 15:12:05, Richard Biener wrote: > > On Tue, Dec 3, 2013 at 2:10 PM, Richard Biener > <richard.guent...@gmail.com> wrote: >> On Tue, Dec 3, 2013 at 1:48 PM, Bernd Edlinger >> <bernd.edlin...@hotmail.de> wrote: >>> Hi Jeff, >>> >>> please find attached the patch (incl. test cases) for the unaligned read >>> BUG that I found while investigating >>> on PR#57748: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57748 >>> >>> one test case is this one: >>> >>> pr57748-3.c: >>> /* PR middle-end/57748 */ >>> /* { dg-do run } */ >>> /* wrong code in expand_expr_real_1. */ >>> >>> #include <stdlib.h> >>> >>> extern void abort (void); >>> >>> typedef long long V >>> __attribute__ ((vector_size (2 * sizeof (long long)), may_alias)); >>> >>> typedef struct S { V a; V b[0]; } P __attribute__((aligned (1))); >>> >>> struct __attribute__((packed)) T { char c; P s; }; >>> >>> void __attribute__((noinline, noclone)) >>> check (P *p) >>> { >>> if (p->b[0][0] != 3 || p->b[0][1] != 4) >>> abort (); >>> } >>> >>> void __attribute__((noinline, noclone)) >>> foo (struct T *t) >>> { >>> V a = { 3, 4 }; >>> t->s.b[0] = a; >>> } >>> >>> int >>> main () >>> { >>> struct T *t = (struct T *) calloc (128, 1); >>> >>> foo (t); >>> check (&t->s); >>> >>> free (t); >>> return 0; >>> } >>> >>> >>> and the other one is >>> pr57748-4.c: >>> /* PR middle-end/57748 */ >>> /* { dg-do run } */ >>> /* wrong code in expand_expr_real_1. */ >>> >>> #include <stdlib.h> >>> >>> extern void abort (void); >>> >>> typedef long long V >>> __attribute__ ((vector_size (2 * sizeof (long long)), may_alias)); >>> >>> typedef struct S { V b[1]; } P __attribute__((aligned (1))); >>> >>> struct __attribute__((packed)) T { char c; P s; }; >>> >>> void __attribute__((noinline, noclone)) >>> check (P *p) >>> { >>> if (p->b[1][0] != 3 || p->b[1][1] != 4) >>> abort (); >>> } >>> >>> void __attribute__((noinline, noclone)) >>> foo (struct T *t) >>> { >>> V a = { 3, 4 }; >>> t->s.b[1] = a; >>> } >>> >>> int >>> main () >>> { >>> struct T *t = (struct T *) calloc (128, 1); >>> >>> foo (t); >>> check (&t->s); >>> >>> free (t); >>> return 0; >>> } >>> >>> >>> The patch does add a boolean "expand_reference" parameter to >>> expand_expr_real and >>> expand_expr_real_1. I pass true when I intend to use the returned memory >>> context >>> as an array reference, instead of a value. At places where mis-aligned >>> values are extracted, >>> I do not return a register with the extracted mis-aligned value if >>> expand_reference is true. >>> When I have a VIEW_CONVERT_EXPR I pay attention to pass down the outer >>> "expand_reference" >>> to the inner expand_expr_real call. Expand_reference, is pretty much >>> similar to the >>> expand_modifier "EXPAND_MEMORY". >>> >>> Boot-strapped and regression-tested on X86_64-pc-linux-gnu (many times). >>> >>> Ok for trunk? >> >> It still feels like papering over the underlying issue. Let me have a >> second (or third?) look. > > Few comments on your patch. > > @@ -9520,6 +9526,7 @@ expand_expr_real_1 (tree exp, rtx target, enum mac > align = get_object_alignment (exp); > if (modifier != EXPAND_WRITE > && modifier != EXPAND_MEMORY > + && !expand_reference > && mode != BLKmode > && align < GET_MODE_ALIGNMENT (mode) > /* If the target does not have special handling for unaligned > > (TARGET_MEM_REF), expand_reference should never be true here, > there may be no component-refs around TARGET_MEM_REFs. >
Ok, I was not sure. Removed - Thanks.
> You miss adjusting the VIEW_CONVERT_EXPR path? (line-numbers
> are off a lot in your patch, context doesn't help very much :/ Does not
> seem to be against 4.8 either ...)
>
Sorry, The line-numbers moved a lot> 100 lines.
The patch is against 4.9-trunk. Re-generated.
> Index: gcc/cfgexpand.c
> ===================================================================
> --- gcc/cfgexpand.c (revision 204411)
> +++ gcc/cfgexpand.c (working copy)
> @@ -2189,7 +2189,7 @@ expand_call_stmt (gimple stmt)
> if (lhs)
> expand_assignment (lhs, exp, false);
> else
> - expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL);
> + expand_expr_real_1 (exp, const0_rtx, VOIDmode, EXPAND_NORMAL, NULL, false);
>
> mark_transaction_restart_calls (stmt);
> }
>
> this should use
>
> expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL);
>
> anyway.
>
expand_expr_real_1 is expand_expr_real without the ERROR check?
Ok, changed to expand_expr.
> @@ -10286,7 +10297,10 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
> op0 = copy_rtx (op0);
> set_mem_align (op0, MAX (MEM_ALIGN (op0), TYPE_ALIGN (type)));
> }
> - else if (mode != BLKmode
> + else if (modifier != EXPAND_WRITE
> + && modifier != EXPAND_MEMORY
> + && !expand_reference
> + && mode != BLKmode
> && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode)
> /* If the target does have special handling for unaligned
> loads of mode then use them. */
>
> @@ -10307,6 +10321,9 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
> return reg;
> }
> else if (STRICT_ALIGNMENT
> + && modifier != EXPAND_WRITE
> + && modifier != EXPAND_MEMORY
> + && !expand_reference
> && mode != BLKmode
> && MEM_ALIGN (op0) < GET_MODE_ALIGNMENT (mode))
> {
>
> why the unrelated change to add the modifier checks? Looks like both
> if cases are close by and factoring out common code like
>
> else if (!expand_reference
> && mode != BLKmode
> && MEM_ALIGN (...
> {
> if (...)
> else if (STRICT_ALIGNMENT)
>
> would be better, also matching the other similar occurances.
>
Ok, re-factored that if cascade.
Thanks.
Bernd.
> Looking for some more time your patch may be indeed the easiest
> without big re-factoring.
>
> Richard.
>
>> Richard.
>>
>>>
>>> Thanks
>>> Bernd. 2013-11-07 Bernd Edlinger <bernd.edlin...@hotmail.de>
PR middle-end/57748
* expr.h (expand_expr_real, expand_expr_real_1): Add new parameter
expand_reference.
(expand_expr, expand_normal): Adjust.
* expr.c (expand_expr_real, expand_expr_real_1): Add new parameter
expand_reference. Use expand_reference to expand inner references.
(store_expr): Adjust.
* cfgexpand.c (expand_call_stmt): Adjust.
testsuite:
2013-11-07 Bernd Edlinger <bernd.edlin...@hotmail.de>
PR middle-end/57748
* gcc.dg/torture/pr57748-3.c: New test.
* gcc.dg/torture/pr57748-4.c: New test.
patch-pr57748-2.diff
Description: Binary data
