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

login
register
mail settings
Submitter Richard Guenther
Date Dec. 3, 2013, 2:12 p.m.
Message ID <CAFiYyc0PgY3miKhjRYAvkWPiCxFc7AOcrN9G=mSL2rS0Qswy1w@mail.gmail.com>
Download mbox | patch
Permalink /patch/296205/
State New
Headers show

Comments

Richard Guenther - Dec. 3, 2013, 2:12 p.m.
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.

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 ...)


this should use

  expand_expr (exp, const0_rtx, VOIDmode, EXPAND_NORMAL);

anyway.

@@ -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.

Looking for some more time your patch may be indeed the easiest
without big re-factoring.

Richard.

> Richard.
>
>>
>> Thanks
>> Bernd.
Bernd Edlinger - Dec. 4, 2013, 8:16 a.m.
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.
2013-11-07  Bernd Edlinger  <bernd.edlinger@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.edlinger@hotmail.de>

	PR middle-end/57748
	* gcc.dg/torture/pr57748-3.c: New test.
	* gcc.dg/torture/pr57748-4.c: New test.
Jeff Law - Dec. 6, 2013, 5:16 a.m.
On 12/04/13 01:16, Bernd Edlinger wrote:

>
>> Looking for some more time your patch may be indeed the easiest
>> without big re-factoring.
Richard (or Bernd), can you comment on why?  Something seems "off" here.

Why do we need to handle inner references here specially?   If feels 
like we're catering to broken code elsewhere in GCC.

As for the patch itself.  In a few places within expand_expr_real_1 you 
changed calls to expand_expr to instead call expand_expr_real.   ISTM 
you could have gotten the same effect by just adding your extra argument 
to the existing code?


Jeff
Bernd Edlinger - Dec. 6, 2013, 8:51 a.m.
Hi,


On Thu, 5 Dec 2013 22:16:15, Jeff Law wrote:
>
> On 12/04/13 01:16, Bernd Edlinger wrote:
>
>>
>>> Looking for some more time your patch may be indeed the easiest
>>> without big re-factoring.
> Richard (or Bernd), can you comment on why? Something seems "off" here.
>
> Why do we need to handle inner references here specially? If feels
> like we're catering to broken code elsewhere in GCC.
>

My first attempt at fixing this was just a one-line change.
like:

@@ -9905,7 +9861,7 @@ expand_expr_real_1 (tree exp, rtx target
                          && modifier != EXPAND_STACK_PARM
                          ? target : NULL_RTX),
                         VOIDmode,
-                        modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
+                        EXPAND_MEMORY);


But the problem with that was, that it might loose too much information...
i.e. little we know what tem might be, and what a change to the original
expand_modifier would have done at other places where we had no
problems at all.

So this current version of the patch is a trying to get as much flexibility
as possible while changing as little as possible at the same time.

Technically we only have to switch off some places in MEM_REF,
and VIEW_CONVERT_EXPR, where an unaligned value is placed in a
register, which turns out to be unnecessary in this special context.



> As for the patch itself. In a few places within expand_expr_real_1 you
> changed calls to expand_expr to instead call expand_expr_real. ISTM
> you could have gotten the same effect by just adding your extra argument
> to the existing code?
>

Yes, but one goal is to keep the patch-file as small as possible,
and expand_expr is used everywhere.

Actually expand_expr is just a wrapper for

expand_expr_real (exp, target, mode, modifier, NULL, false)

Therefore I replaced expand_expr with expand_expr_real at these few
places, to keep the change smaller.

Another point is, that I do not want to pass true for expand_reference
at any other place than from exand_expr_real_1 where the inner
reference is expanded.


Thanks
Bernd.

>
> Jeff
Richard Guenther - Dec. 6, 2013, 10:06 a.m.
On Fri, Dec 6, 2013 at 6:16 AM, Jeff Law <law@redhat.com> wrote:
> On 12/04/13 01:16, Bernd Edlinger wrote:
>
>>
>>> Looking for some more time your patch may be indeed the easiest
>>> without big re-factoring.
>
> Richard (or Bernd), can you comment on why?  Something seems "off" here.
>
> Why do we need to handle inner references here specially?   If feels like
> we're catering to broken code elsewhere in GCC.

The issue is that we handle expansion of misaligned moves in the code
we recurse to - but that misaligned move handling can only work at
the "outermost" level of the recursion as it is supposed to see the
"real" access (alignment and mode of the memory access, not of
some base of it).

So we need a recursion that skips this part (and others - which already
works), just processing as-if in "expand the base of some memory access"
mode.

That we recurse at all when expanding memory accesses makes this
expansion path quite a twisted maze - which is why I suggested to
re-factor the whole thing to not require recursion (but that will be
a very big change, not suitable for this stage).

The easiest is to add a flag to indicate the "we're-expanding-the-base",
doing another expand modifier doesn't work as they are not combinable
and the passed modifier is already modified for the recursion - and that
dependent on stuff.

"Fixing" the mode of the base object isn't really fixing the fact that
the recursion shouldn't even try to generate a movmisalign mem,
it just papers over this issue by making it (hopefully) never trigger
(at least for valid code) for bases.

Richard.

> As for the patch itself.  In a few places within expand_expr_real_1 you
> changed calls to expand_expr to instead call expand_expr_real.   ISTM you
> could have gotten the same effect by just adding your extra argument to the
> existing code?
>
>
> Jeff
Jeff Law - Dec. 6, 2013, 9:17 p.m.
On 12/06/13 03:06, Richard Biener wrote:
>
> The issue is that we handle expansion of misaligned moves in the code
> we recurse to - but that misaligned move handling can only work at
> the "outermost" level of the recursion as it is supposed to see the
> "real" access (alignment and mode of the memory access, not of
> some base of it).
>
> So we need a recursion that skips this part (and others - which already
> works), just processing as-if in "expand the base of some memory access"
> mode.
So it's really not a case of someone outside expand_expr needing 
different behaviour, but a case of stopping the recursion within 
expand_expr.  That's a bit less concerning.

>
> That we recurse at all when expanding memory accesses makes this
> expansion path quite a twisted maze - which is why I suggested to
> re-factor the whole thing to not require recursion (but that will be
> a very big change, not suitable for this stage).
No doubt.  In general expand_expr is a mess and has been, well, forever.


>
> The easiest is to add a flag to indicate the "we're-expanding-the-base",
> doing another expand modifier doesn't work as they are not combinable
> and the passed modifier is already modified for the recursion - and that
> dependent on stuff.
We could always make the modifiers a bitmask, but probably not something 
we really need to do during stage3.

>
> "Fixing" the mode of the base object isn't really fixing the fact that
> the recursion shouldn't even try to generate a movmisalign mem,
> it just papers over this issue by making it (hopefully) never trigger
> (at least for valid code) for bases.
Ok, that answers the other question I had after looking at other parts 
of this thread.  Though one could argue that the modes are in fact wrong.

Jeff
Jeff Law - Dec. 6, 2013, 9:21 p.m.
On 12/06/13 01:51, Bernd Edlinger wrote:
>>> As for the patch itself. In a few places within expand_expr_real_1 you
>> changed calls to expand_expr to instead call expand_expr_real. ISTM
>> you could have gotten the same effect by just adding your extra argument
>> to the existing code?
>>
>
> Yes, but one goal is to keep the patch-file as small as possible,
> and expand_expr is used everywhere.
>
> Actually expand_expr is just a wrapper for
>
> expand_expr_real (exp, target, mode, modifier, NULL, false)
Sorry, I glossed over that -- mentally I saw the change to expand_expr 
and assumed you passed the new flag through.  But that's not how the 
change is implemented.  My bad.

So clearly for the cases where you want the flag to be true, you can't 
go through the wrapper.  Again, my bad.


jeff

Patch

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);
 }