This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH] Invalid unpoisoning of stack redzones on ARM
- From: Richard Sandiford <rsandifo at linux dot vnet dot ibm dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: Yury Gribov <y dot gribov at samsung dot com>, gcc-patches at gcc dot gnu dot org, dodji at gcc dot gnu dot org, dvyukov at gcc dot gnu dot org, kcc at gcc dot gnu dot org, GarbuzovViacheslav <v dot garbuzov at samsung dot com>, Evgeny Gavrin <e dot gavrin at samsung dot com>
- Date: Tue, 29 Oct 2013 13:06:21 +0000
- Subject: Re: [PATCH] Invalid unpoisoning of stack redzones on ARM
- Authentication-results: sourceware.org; auth=none
- References: <524591E1 dot 9060302 at samsung dot com> <20130927142529 dot GN30970 at tucnak dot zalov dot cz> <5248FBF4 dot 2050807 at samsung dot com> <525240F7 dot 1010409 at samsung dot com> <525E2599 dot 4020803 at samsung dot com> <20131029093730 dot GV30970 at tucnak dot zalov dot cz> <87bo28z2ki dot fsf at sandifor-thinkpad dot stglab dot manchester dot uk dot ibm dot com> <20131029124116 dot GZ30970 at tucnak dot zalov dot cz>
Jakub Jelinek <firstname.lastname@example.org> writes:
> On Tue, Oct 29, 2013 at 12:25:33PM +0000, Richard Sandiford wrote:
>> >> Any updates on this one? Note that this bug is a huge blocker for
>> >> using AddressSanitizer on ARM platforms.
>> > Sorry for the delay, I finally found time to look at it.
>> > While your patch fixes the issue, I wonder if for the case where
>> > shadow_mem before the asan_clear_shadow call is already of the form
>> > (mem (reg)) it isn't better to just adjust next asan_clear_shadow
>> > base from the end of the cleared region rather than from the start of it,
>> > because the end will be already in the right pseudo, while with your
>> > approach it needs to be done in a different register and potentially
>> > increase register pressure. So here is (completely untested) alternate fix:
>> Hmm, it just seems wrong to be assigning to registers returned by force_reg
>> and expand_binop though. Would this variation be OK?
Well, that was the traditional rule:
The caller must not alter the value in the register we return,
since we mark it as a "constant" register. */
force_reg (enum machine_mode mode, rtx x)
but maybe it doesn't matter now, since the constant register flag has
gone and since we seem to use REG_EQUAL rather than REG_EQUIV...
> If it is a pseudo, it is certainly a pseudo that isn't used for
> anything else, as it is the result of (base >> 3) + constant, if it isn't a
> pseudo, then supposedly it is better not to just keep adding the offsets to
> a base and thus not to use the end address from asan_clear_shadow.
> Your patch would force using the end address even if shadow_base
> is initially say some_reg + 32, I think it is better to use shadow_mem
> some_reg + 72 next time instead of some_other_reg + 40.
But I thought the register pressure thing was to avoid having the
original base live across the loop. Doesn't that apply for reg + offset
as well as plain reg? If we have to force some_reg + 32 into a
temporary and then loop on it, using the new base should avoid keeping
both it and some_reg live at the same time. Plus smaller offsets are
often better on some targets.