If you don't mind my realigning on STRICT_ALIGNMENT machines,I will make
patch v2 soon.
>> On Thu, Feb 20, 2014 at 04:19:12PM +0800, lin zuojian wrote:
>>> Without aligning the asan stack base,base will only 64-bit aligned in
>>> ARM machines.
>>> But asan require 256-bit aligned base because of this:
>>> 1.right shift take ASAN_SHADOW_SHIFT(which is 3) bits are zeros
>>> 2.store multiple/load multiple instructions require the other 2 bits are
>>> zeros
>>>
>>> that add up lowest 5 bits should be zeros.That means 32 bytes or 256
>>> bits aligned.
>>>
>>> * asan.c (asan_emit_stack_protection): Forcing the base to align to 256 bits
>>> * cfgexpand.c (expand_used_vars): Leaving a space in the stack frame for
>>> alignment
>> This is wrong.
>>
>> You certainly don't want to do any of the extra aligning
>> on non-strict alignment targets, so all the changes must be
>> if (STRICT_ALIGNMENT) guarded, as they are quite expensive (you need one
>> extra register in that case).
> I have to do realignment.And llvm's backend also do the realignment.I
> just refers to its implementation.
>
>> Second thing is, I think you break --param use-after-return=0 this way
>> and also functions with very large stack frames, because emit_move_insn
>> to pbase is then done before you adjust the stack.
> Yes, that's a defect.
>> Also, I don't think you want to align for ASAN_RED_ZONE_SIZE,
>> but instead (GET_MODE_ALIGNMENT (SImode) << ASAN_SHADOW_SHIFT) /
>> BITS_PER_UNIT,
>> and if you do (of course for STRICT_ALIGNMENT targets only), you should
>> set_mem_align on shadow_mem when it is created.
> I agree with you on the alignment value.May invoking set_mem_align on
> shadow_mem happen too late at this point?Given RTL has been expanded.
> And again I have to do realignment on non STRICT_ALIGNMENT whatever cost
> it take or there are always alignment faults.
>
>> If you do the realignment, then it would be nice if the middle-end was told
>> about that, so do
>> base_align = crtl->max_used_stack_slot_alignment;
>> in both the if ((flag_sanitize & SANITIZE_ADDRESS) && ASAN_STACK && pred)
>> then body and else, and if sanitizing and STRICT_ALIGNMENT, set
>> base_align to MIN of crtl->max_used_stack_slot_alignment and the alignment
>> you actually guarantee.
> Okay,I will do that.
>
>> Or, if ARM supports unaligned loads/stores using special instructions,
>> perhaps you should also benchmark the alternative of not realigning, but
>> instead making sure those unaligned instructions are used for the shadow
>> memory loads/stores in the asan prologue/epilogue.
> I have tried to use -fno-peephole2 to shutdown instructions
> combinations,but that makes me feel uncomfortable.
>> Please next time post the patch from a sane MUA that doesn't eat tabs or
>> at least as an attachment. Comments should start with a capital letter, and
>> with a full stop followed by two spaces, in the ChangeLog also all entries
>> should end with a full stop.
> Sorry about that...
> BTW,I didn't modify the ChangeLog because this patch should be discussed
> carefully.
>> Jakub
>
