This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Richard Biener <richard dot guenther at gmail dot com>, Bernd Edlinger <bernd dot edlinger at hotmail dot de>, Eric Botcazou <ebotcazou at adacore dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 25 Sep 2013 11:05:21 +0200
- Subject: Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
- Authentication-results: sourceware.org; auth=none
- References: <DUB122-W4D571A91831593BA22438E43C0 at phx dot gbl> <CAFiYyc1ECAdBJTkcziSL_f9SQyYUPY9k19QWqkN+RfLO=i2zaQ at mail dot gmail dot com> <20130910193228 dot GE6732 at virgil dot suse> <DUB122-W22DAEEE94B5ED3308F6967E4390 at phx dot gbl> <CAFiYyc3AHggjJbEmZvC_fh67o+LVpTBRdwyUv8T6EhQFE2vNWQ at mail dot gmail dot com> <DUB122-W281E996A4885309956418EE4250 at phx dot gbl> <CAFiYyc37ok7hde0XgiKV2uQPnOg6VzbS8QfDBjCccbvkwer54g at mail dot gmail dot com> <CAFiYyc2SgSvVqq412Bx2yA1d7fM3RTFmHvXvUobs+jp-hUh3UA at mail dot gmail dot com> <DUB122-W47C123A809B4FBF5146BE6E42E0 at phx dot gbl> <CAFiYyc3Rr0emS0OeEu7+gYvtmO2-754CLjqqMUtrwgUE5U8Dqw at mail dot gmail dot com> <20130924180651 dot GB21731 at virgil dot suse>
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
- References:
- Re: [PATCH, PR 57748] Check for out of bounds access
- Re: [PATCH, PR 57748] Check for out of bounds access
- RE: [PATCH, PR 57748] Check for out of bounds access
- Re: [PATCH, PR 57748] Check for out of bounds access
- RE: [PATCH, PR 57748] Check for out of bounds access
- Re: [PATCH, PR 57748] Check for out of bounds access
- Re: [PATCH, PR 57748] Check for out of bounds access
- [PATCH, PR 57748] Check for out of bounds access, Part 2
- Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
- Re: [PATCH, PR 57748] Check for out of bounds access, Part 2