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

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.

So I believe the issue is that we are asking the target for an optab
for movmisaling with the wrong mode and then create a movmisalign
with a wrong mode.


fixes the testcases (the VIEW_CONVERT_EXPR path would need a
similar fix I think, both for the recursion and the movmisaling handling).
Note that the alignment checks guarding the movmisalign handling
use the wrong mode as well, they are also somewhat non-sensical
as they apply to the non-offsetted object.

That said, the "get me a 'base' MEM for this" via recursing is misdesigned.
I still think that splitting it out is the correct fix in the end (doing the
movmisaling handling only there, but with correct info from the parent).

I will try if I can come up with something that feels reasonably safe
for stage3.

Richard.

> Richard.
>
>>
>> Thanks
>> Bernd.

----------------------------------------
> Date: Tue, 3 Dec 2013 14:51:21 +0100
> Subject: Re: [REPOST] Invalid Code when reading from unaligned zero-sized array
> From: richard.guenther@gmail.com
> To: bernd.edlinger@hotmail.de
> CC: law@redhat.com; gcc-patches@gcc.gnu.org; jakub@redhat.com
>
> 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.
>
> So I believe the issue is that we are asking the target for an optab
> for movmisaling with the wrong mode and then create a movmisalign
> with a wrong mode.
>
> Index: gcc/expr.c
> ===================================================================
> --- gcc/expr.c (revision 205623)
> +++ gcc/expr.c (working copy)
> @@ -9737,7 +9737,7 @@ expand_expr_real_1 (tree exp, rtx target
> && mode != BLKmode
> && align < GET_MODE_ALIGNMENT (mode))
> {
> - if ((icode = optab_handler (movmisalign_optab, mode))
> + if ((icode = optab_handler (movmisalign_optab, tmode))
> != CODE_FOR_nothing)
> {
> struct expand_operand ops[2];
> @@ -9745,7 +9745,7 @@ expand_expr_real_1 (tree exp, rtx target
> /* We've already validated the memory, and we're creating a
> new pseudo destination. The predicates really can't fail,
> nor can the generator. */
> - create_output_operand (&ops[0], NULL_RTX, mode);
> + create_output_operand (&ops[0], NULL_RTX, tmode);
> create_fixed_operand (&ops[1], temp);
> expand_insn (icode, 2, ops);
> temp = ops[0].value;
> @@ -9966,7 +9966,7 @@ expand_expr_real_1 (tree exp, rtx target
> != INTEGER_CST)
> && modifier != EXPAND_STACK_PARM
> ? target : NULL_RTX),
> - VOIDmode,
> + mode1,
> modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
>
> /* If the bitfield is volatile, we want to access it in the
>
> fixes the testcases (the VIEW_CONVERT_EXPR path would need a
> similar fix I think, both for the recursion and the movmisaling handling).
> Note that the alignment checks guarding the movmisalign handling
> use the wrong mode as well, they are also somewhat non-sensical
> as they apply to the non-offsetted object.
>
> That said, the "get me a 'base' MEM for this" via recursing is misdesigned.
> I still think that splitting it out is the correct fix in the end (doing the
> movmisaling handling only there, but with correct info from the parent).
>
> I will try if I can come up with something that feels reasonably safe
> for stage3.
>

Fine. What is tmode and mode in the test example?

Note: we can run in the same problem if this is executed:

            else if (SLOW_UNALIGNED_ACCESS (mode, align))
              temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
                                        0, TYPE_UNSIGNED (TREE_TYPE (exp)),
                                        (modifier == EXPAND_STACK_PARM
                                         ? NULL_RTX : target),
                                        mode, mode);

maybe with %s/mode/tmode/g

Maybe just #define SLOW_UNALIGNED_ACCESS 1 on X86-64 to see
what might happen on a different target....

Thanks
Bernd.