> 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
