Re: [PATCH, PR 57748] Check for out of bounds access, Part 2

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Sep 24, 2013 at 8:06 PM, Martin Jambor <mjambor@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.edlinger@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.

So what we maybe need is another expand modifier that tells us
to not use movmisalign when expanding the base object.  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.  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.

Richard.

> Martin