Hi,
On Wed, 25 Sep 2013 16:01:02, Martin Jambor wrote:
> Hi,
>
> On Wed, Sep 25, 2013 at 11:05:21AM +0200, Richard Biener wrote:
>> On Tue, Sep 24, 2013 at 8:06 PM, Martin Jambor <mjam...@suse.cz> wrote:
>>> Hi,
>>>
>>> On Tue, Sep 24, 2013 at 12:02:17PM +0200, Richard Biener wrote:
>>>> On Tue, Sep 24, 2013 at 4:52 AM, Bernd Edlinger
>>>> <bernd.edlin...@hotmail.de> wrote:
>>>>> Hi,
>>>>>
>>>>> with the attached patch the read-side of the out of bounds accesses are
>>>>> fixed.
>>>>> There is a single new test case pr57748-3.c that is derived from Martin's
>>>>> test case.
>>>>> The test case does only test the read access and does not depend on part
>>>>> 1 of the
>>>>> patch.
>>>>>
>>>>> This patch was boot-strapped and regression tested on
>>>>> x86_64-unknown-linux-gnu.
>>>>>
>>>>> Additionally I generated eCos and an eCos-application (on ARMv5 using
>>>>> packed
>>>>> structures) with an arm-eabi cross compiler, and looked for differences
>>>>> in the
>>>>> disassembled code with and without this patch, but there were none.
>>>>>
>>>>> OK for trunk?
>>>>
>>>> Index: gcc/expr.c
>>>> ===================================================================
>>>> --- gcc/expr.c (revision 202778)
>>>> +++ gcc/expr.c (working copy)
>>>> @@ -9878,7 +9878,7 @@
>>>> && modifier != EXPAND_STACK_PARM
>>>> ? target : NULL_RTX),
>>>> VOIDmode,
>>>> - modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
>>>> + EXPAND_MEMORY);
>>>>
>>>> /* If the bitfield is volatile, we want to access it in the
>>>> field's mode, not the computed mode.
>>>>
>>>> context suggests that we may arrive with EXPAND_STACK_PARM here
>>>> which is a correctness modifier (see its docs). But I'm not too familiar
>>>> with the details of the various expand modifiers, Eric may be though.
>>>>
>>>> That said, I still believe that fixing the misalign path in
>>>> expand_assignment
>>>> would be better than trying to avoid it. For this testcase the issue is
>>>> again that expand_assignment passes the wrong mode/target to the
>>>> movmisalign optab.
>>>
>>> Perhaps I'm stating the obvious, but this is supposed to address a
>>> separate bug in the expansion of the RHS (as opposed to the first bug
>>> which was in the expansion of the LHS), and so we would have to make
>>> expand_assignment to also examine potential misalignments in the RHS,
>>> which it so far does not do, despite its complexity.
>>>
>>> Having said that, I also dislike the patch, but I am quite convinced
>>> that if we allow non-BLK structures with zero sized arrays, the fix
>>> will be ugly - but I'll be glad to be shown I've been wrong.
>>
>> I don't think the issues have anything to do with zero sized arrays
>> or non-BLK structures. The issue is just we are feeding movmisaling
>> with the wrong mode (the mode of the base object) if we are expanding
>> a base object which is unaligned and has non-BLK mode.
>
> I disagree. Consider a slightly modified testcase:
>
> #include <stdlib.h>
> typedef long long V
> __attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
>
> #if 1
> typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
> struct __attribute__((packed)) T { char c; P s; };
> #else
> typedef struct S { V a; V b[0]; } P;
> struct T { char c; P s; };
> #endif
>
> void __attribute__((noinline, noclone))
> good_value (V b)
> {
> if (b[0] != 3 || b[1] != 4)
> __builtin_abort ();
> }
>
> void __attribute__((noinline, noclone))
> check (P *p)
> {
> good_value (p->b[0]);
> }
>
> int
> main ()
> {
> struct T *t = (struct T *) calloc (128, 1);
> V a = { 3, 4 };
> t->s.b[0] = a;
> check (&t->s);
> free (t);
> return 0;
> }
>
> The problem is the expansion of the memory load in function check.
> All involved modes, the one of the base structure and of the loaded
> component, are V2DI so even if we somehow mixed them up, in this
> example it should not matter, yet the unaligned load gives wrong
> results.
>
> I'm still convinced that the problem is that we have a V2DImode
> structure which is larger that the mode tells and we want to load the
> data from outside of its mode. That is only happening because zero
> sized arrays.
Yes, this is another good example, and it passes vector values on the
stack to a function. Again my patch produces working code, while
the current trunk produces just completely _insane_ code.
And again, this is not only a problem of structures with zero-sized
arrays at the end. Remember my previous example code:
On ARM (or anything with STRICT_ALIGNMENT) this union has the
same problems:
/* PR middle-end/57748 */
/* arm-eabi-gcc -mcpu=cortex-a9 -O3 */
#include <stdlib.h>
union x
{
short a[2];
char x[4];
} __attribute__((packed, aligned(4))) ;
typedef volatile union x *s;
void __attribute__((noinline, noclone))
check (void)
{
s xx=(s)(0x80000002);
/* although volatile xx->x[3] reads 4 bytes here */
if (xx->x[3] != 3)
abort ();
}
void __attribute__((noinline, noclone))
foo (void)
{
s xx=(s)(0x80000002);
xx->x[3] = 3;
}
int
main ()
{
foo ();
check ();
return 0;
}
>>
>> So what we maybe need is another expand modifier that tells us
>> to not use movmisalign when expanding the base object.
>
> In that case we can just as well use EXPAND_MEMORY. If so, I'd do
> that only when there is a zero sized array involved in order not to
> avoid using movmisalign when we can.
>
>> Or we need to stop expanding the base object with VOIDmode and
>> instead pass down the mode of the access (TYPE_MODE (TREE_TYPE
>> (exp)) which will probably confuse other code.
>
> Well, because I think the problem is not (just) mixing up modes, I
> don't think this will help either.
>
>> So eventually not
>> handling the misaligned case by expansion of the base but inlined
>> similar to how it was in expand_assignment would be needed here.
>
> At least now I fail to see how this would be different from copying a
> large portion of expand_expr_real_1 (all of the MEM_REF part without
> the movmisalign bit) and calling that when we detect "this case"
> whatever we eventually agree it is.
>
> Martin
So I still think my patch does the right thing.
The rationale is:
= expand_expr (tem,
(TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
&& COMPLETE_TYPE_P (TREE_TYPE (tem))
&& (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem)))
!= INTEGER_CST)
&& modifier != EXPAND_STACK_PARM
? target : NULL_RTX),
VOIDmode,
EXPAND_MEMORY);
returns the address of the structure in question,
we can add offset, bitoffset, and access the memory
in the right mode and alignment information is
passed to the backend via MEM_ALIGN (op0).
Passing EXPAND_STACK_PARM would explicitly do the
wrong thing, as this is not the final result but only an
intermediate value.
As long as MEM_ALIGN(op0) is right it does not
matter if we use mov_optab or movmisalign_optab.
If _that_ is a bug, then the back-ends worked around it
already for years, and I don't see any reason for changing
that now.
Bernd.
