This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [REPOST] Invalid Code when reading from unaligned zero-sized array
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Bernd Edlinger <bernd dot edlinger at hotmail dot de>
- Cc: Jeff Law <law at redhat dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, Jakub Jelinek <jakub at redhat dot com>
- Date: Tue, 3 Dec 2013 15:12:05 +0100
- Subject: Re: [REPOST] Invalid Code when reading from unaligned zero-sized array
- Authentication-results: sourceware.org; auth=none
- References: <DUB122-W20B5CD3C7B50824A33E21AE4D50 at phx dot gbl> <CAFiYyc17fikBtmMsMGk7CsQhGWU430iku=PBpLyrQEsLBo5CuQ at mail dot gmail dot 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.
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 ...)
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.
@@ -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.