[REPOST] Invalid Code when reading from unaligned zero-sized array

[Bernd Edlinger]

On Tue, 3 Dec 2013 15:12:05, Richard Biener wrote:
>
> On Tue, Dec 3, 2013 at 2:10 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Tue, Dec 3, 2013 at 1:48 PM, Bernd Edlinger
>> <bernd.edlinger@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. 		 	   		  
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: changelog-pr57748-2.txt
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20131204/d2c7befa/attachment.txt>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch-pr57748-2.diff
Type: application/octet-stream
Size: 12746 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20131204/d2c7befa/attachment.obj>