<div dir="ltr">Hi Hal, <div><br></div><div>Values of <span style="font-family:arial,sans-serif;font-size:13px">SHADOW_GRANULARITY other than 8 are currently completely untested, </span></div><div><span style="font-family:arial,sans-serif;font-size:13px">so it's no surprise that some of the changes broke with </span><span style="font-family:arial,sans-serif;font-size:13px">SHADOW_GRANULARITY != 8.</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">If we want to revive other values, we also need to set up testing for them.</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">I am very curious, why are you using larger granularity? </span></div><div><span style="font-family:arial,sans-serif;font-size:13px">Larger granularity gives us more compact shadow (which is good) but it also increases the minimal </span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">possible redzone (which is not that good). Transition from granularity=8 to any larger one</span></div><div><span style="font-family:arial,sans-serif;font-size:13px">makes instrumentation of 64-bit loads more expensive (not good at all). </span></div>
<div><br></div><div>As for the patches you propose, may I ask you to send them using <a href="http://llvm.org/docs/Phabricator.html">http://llvm.org/docs/Phabricator.html</a> ? </div><div><br></div><div>Thanks, </div><div>
<br></div><div>--kcc </div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Sat, Jul 6, 2013 at 3:14 AM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">----- Original Message -----<br>
> Author: kcc<br>
> Date: Wed Dec 26 04:41:24 2012<br>
> New Revision: 171107<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=171107&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=171107&view=rev</a><br>
> Log:<br>
> [asan] asan_allocator2: implement adaptive redzones. Now with<br>
> asan_allocator2 allocations <= 48 bytes have 16 byte redzone,<br>
> allocations of 48-96 bytes -- have 32 bytes redzone, etc (max<br>
> redzone is 2048). If ASAN_OPTIONS=redzone=n is set, it changes the<br>
> minimal redzone size<br>
<br>
</div>I'm seeing a small problem with this in the case where SHADOW_GRANULARITY is greater than 8. If I set it to 32 then, for small allocations, the CHECK(IsAligned(needed_size, min_alignment)); in Allocate fails.<br>
<br>
Would it make sense to replace:<br>
u32 rz_log =<br>
<div class="im"> user_requested_size <= 64 - 16 ? 0 :<br>
</div><div class="im"> user_requested_size <= 128 - 32 ? 1 :<br>
</div><div class="im"> user_requested_size <= 512 - 64 ? 2 :<br>
</div>...<br>
<br>
with something like:<br>
u32 rz_log =<br>
SHADOW_GRANULARITY <= 8 && user_requested_size <= 64 - 16 ? 0 :<br>
SHADOW_GRANULARITY <= 16 && user_requested_size <= 128 - 32 ? 1 :<br>
SHADOW_GRANULARITY <= 32 && user_requested_size <= 512 - 64 ? 2 :<br>
...<br>
?<br>
<br>
-Hal<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
> Modified:<br>
> compiler-rt/trunk/lib/asan/asan_allocator2.cc<br>
> compiler-rt/trunk/lib/asan/asan_rtl.cc<br>
> compiler-rt/trunk/lib/asan/tests/asan_test.cc<br>
><br>
> Modified: compiler-rt/trunk/lib/asan/asan_allocator2.cc<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/asan/asan_allocator2.cc?rev=171107&r1=171106&r2=171107&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/asan/asan_allocator2.cc?rev=171107&r1=171106&r2=171107&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/asan/asan_allocator2.cc (original)<br>
> +++ compiler-rt/trunk/lib/asan/asan_allocator2.cc Wed Dec 26 04:41:24<br>
> 2012<br>
> @@ -99,11 +99,39 @@<br>
> // CHUNK_ALLOCATED: the chunk is allocated and not yet freed.<br>
> // CHUNK_QUARANTINE: the chunk was freed and put into quarantine<br>
> zone.<br>
> enum {<br>
> - CHUNK_AVAILABLE = 1,<br>
> + CHUNK_AVAILABLE = 0, // 0 is the default value even if we didn't<br>
> set it.<br>
> CHUNK_ALLOCATED = 2,<br>
> CHUNK_QUARANTINE = 3<br>
> };<br>
><br>
> +// Valid redzone sizes are 16, 32, 64, ... 2048, so we encode them<br>
> in 3 bits.<br>
> +// We use adaptive redzones: for larger allocation larger redzones<br>
> are used.<br>
> +static u32 RZLog2Size(u32 rz_log) {<br>
> + CHECK_LT(rz_log, 8);<br>
> + return 16 << rz_log;<br>
> +}<br>
> +<br>
> +static u32 RZSize2Log(u32 rz_size) {<br>
> + CHECK_GE(rz_size, 16);<br>
> + CHECK_LE(rz_size, 2048);<br>
> + CHECK(IsPowerOfTwo(rz_size));<br>
> + u32 res = __builtin_ctz(rz_size) - 4;<br>
> + CHECK_EQ(rz_size, RZLog2Size(res));<br>
> + return res;<br>
> +}<br>
> +<br>
> +static uptr ComputeRZLog(uptr user_requested_size) {<br>
> + u32 rz_log =<br>
> + user_requested_size <= 64 - 16 ? 0 :<br>
> + user_requested_size <= 128 - 32 ? 1 :<br>
> + user_requested_size <= 512 - 64 ? 2 :<br>
> + user_requested_size <= 4096 - 128 ? 3 :<br>
> + user_requested_size <= (1 << 14) - 256 ? 4 :<br>
> + user_requested_size <= (1 << 15) - 512 ? 5 :<br>
> + user_requested_size <= (1 << 16) - 1024 ? 6 : 7;<br>
> + return Max(rz_log, RZSize2Log(flags()->redzone));<br>
> +}<br>
> +<br>
> // The memory chunk allocated from the underlying allocator looks<br>
> like this:<br>
> // L L L L L L H H U U U U U U R R<br>
> // L -- left redzone words (0 or more bytes)<br>
> @@ -130,6 +158,7 @@<br>
> u32 free_tid : 24;<br>
> u32 from_memalign : 1;<br>
> u32 alloc_type : 2;<br>
> + u32 rz_log : 3;<br>
> // 2-nd 8 bytes<br>
> // This field is used for small sizes. For large sizes it is equal<br>
> to<br>
> // SizeClassMap::kMaxSize and the actual size is stored in the<br>
> @@ -149,11 +178,6 @@<br>
> COMPILER_CHECK(kChunkHeaderSize == 16);<br>
> COMPILER_CHECK(kChunkHeader2Size <= 16);<br>
><br>
> -static uptr ComputeRZSize(uptr user_requested_size) {<br>
> - // FIXME: implement adaptive redzones.<br>
> - return flags()->redzone;<br>
> -}<br>
> -<br>
> struct AsanChunk: ChunkBase {<br>
> uptr Beg() { return reinterpret_cast<uptr>(this) +<br>
> kChunkHeaderSize; }<br>
> uptr UsedSize() {<br>
> @@ -164,21 +188,22 @@<br>
> void *AllocBeg() {<br>
> if (from_memalign)<br>
> return allocator.GetBlockBegin(reinterpret_cast<void<br>
> *>(this));<br>
> - return reinterpret_cast<void*>(Beg() - ComputeRZSize(0));<br>
> + return reinterpret_cast<void*>(Beg() - RZLog2Size(rz_log));<br>
> }<br>
> // We store the alloc/free stack traces in the chunk itself.<br>
> u32 *AllocStackBeg() {<br>
> - return (u32*)(Beg() - ComputeRZSize(UsedSize()));<br>
> + return (u32*)(Beg() - RZLog2Size(rz_log));<br>
> }<br>
> uptr AllocStackSize() {<br>
> - return (ComputeRZSize(UsedSize()) - kChunkHeaderSize) /<br>
> sizeof(u32);<br>
> + CHECK_LE(RZLog2Size(rz_log), kChunkHeaderSize);<br>
> + return (RZLog2Size(rz_log) - kChunkHeaderSize) / sizeof(u32);<br>
> }<br>
> u32 *FreeStackBeg() {<br>
> return (u32*)(Beg() + kChunkHeader2Size);<br>
> }<br>
> uptr FreeStackSize() {<br>
> uptr available = Max(RoundUpTo(UsedSize(), SHADOW_GRANULARITY),<br>
> - ComputeRZSize(UsedSize()));<br>
> + (uptr)RZLog2Size(rz_log));<br>
> return (available - kChunkHeader2Size) / sizeof(u32);<br>
> }<br>
> };<br>
> @@ -246,10 +271,8 @@<br>
> PoisonShadow(m->Beg(),<br>
> RoundUpTo(m->UsedSize(), SHADOW_GRANULARITY),<br>
> kAsanHeapLeftRedzoneMagic);<br>
> - uptr alloc_beg = m->Beg() - ComputeRZSize(m->UsedSize());<br>
> - void *p = reinterpret_cast<void *>(alloc_beg);<br>
> + void *p = reinterpret_cast<void *>(m->AllocBeg());<br>
> if (m->from_memalign) {<br>
> - p = allocator.GetBlockBegin(p);<br>
> uptr *memalign_magic = reinterpret_cast<uptr *>(p);<br>
> CHECK_EQ(memalign_magic[0], kMemalignMagic);<br>
> CHECK_EQ(memalign_magic[1], reinterpret_cast<uptr>(m));<br>
> @@ -302,7 +325,8 @@<br>
> return 0; // 0 bytes with large alignment requested. Just<br>
> return 0.<br>
> }<br>
> CHECK(IsPowerOfTwo(alignment));<br>
> - uptr rz_size = ComputeRZSize(size);<br>
> + uptr rz_log = ComputeRZLog(size);<br>
> + uptr rz_size = RZLog2Size(rz_log);<br>
> uptr rounded_size = RoundUpTo(size, rz_size);<br>
> uptr needed_size = rounded_size + rz_size;<br>
> if (alignment > rz_size)<br>
> @@ -336,6 +360,7 @@<br>
> AsanChunk *m = reinterpret_cast<AsanChunk *>(chunk_beg);<br>
> m->chunk_state = CHUNK_ALLOCATED;<br>
> m->alloc_type = alloc_type;<br>
> + m->rz_log = rz_log;<br>
> u32 alloc_tid = t ? t->tid() : 0;<br>
> m->alloc_tid = alloc_tid;<br>
> CHECK_EQ(alloc_tid, m->alloc_tid); // Does alloc_tid fit into the<br>
> bitfield?<br>
> @@ -354,7 +379,9 @@<br>
> } else {<br>
> CHECK(!allocator.FromPrimary(allocated));<br>
> m->user_requested_size = SizeClassMap::kMaxSize;<br>
> - *reinterpret_cast<uptr *>(allocator.GetMetaData(allocated)) =<br>
> size;<br>
> + uptr *meta = reinterpret_cast<uptr<br>
> *>(allocator.GetMetaData(allocated));<br>
> + meta[0] = size;<br>
> + meta[1] = chunk_beg;<br>
> }<br>
><br>
> if (flags()->use_stack_depot) {<br>
> @@ -465,17 +492,34 @@<br>
> }<br>
><br>
> static AsanChunk *GetAsanChunkByAddr(uptr p) {<br>
> - uptr alloc_beg = reinterpret_cast<uptr>(<br>
> - allocator.GetBlockBegin(reinterpret_cast<void *>(p)));<br>
> + void *ptr = reinterpret_cast<void *>(p);<br>
> + uptr alloc_beg =<br>
> reinterpret_cast<uptr>(allocator.GetBlockBegin(ptr));<br>
> if (!alloc_beg) return 0;<br>
> uptr *memalign_magic = reinterpret_cast<uptr *>(alloc_beg);<br>
> if (memalign_magic[0] == kMemalignMagic) {<br>
> - AsanChunk *m = reinterpret_cast<AsanChunk<br>
> *>(memalign_magic[1]);<br>
> - CHECK(m->from_memalign);<br>
> - return m;<br>
> + AsanChunk *m = reinterpret_cast<AsanChunk *>(memalign_magic[1]);<br>
> + CHECK(m->from_memalign);<br>
> + return m;<br>
> + }<br>
> + if (!allocator.FromPrimary(ptr)) {<br>
> + uptr *meta = reinterpret_cast<uptr *>(<br>
> + allocator.GetMetaData(reinterpret_cast<void *>(alloc_beg)));<br>
> + AsanChunk *m = reinterpret_cast<AsanChunk *>(meta[1]);<br>
> + return m;<br>
> + }<br>
> + uptr actual_size = allocator.GetActuallyAllocatedSize(ptr);<br>
> + CHECK_LE(actual_size, SizeClassMap::kMaxSize);<br>
> + // We know the actually allocted size, but we don't know the<br>
> redzone size.<br>
> + // Just try all possible redzone sizes.<br>
> + for (u32 rz_log = 0; rz_log < 8; rz_log++) {<br>
> + u32 rz_size = RZLog2Size(rz_log);<br>
> + uptr max_possible_size = actual_size - rz_size;<br>
> + if (ComputeRZLog(max_possible_size) != rz_log)<br>
> + continue;<br>
> + return reinterpret_cast<AsanChunk *>(<br>
> + alloc_beg + rz_size - kChunkHeaderSize);<br>
> }<br>
> - uptr chunk_beg = alloc_beg + ComputeRZSize(0) - kChunkHeaderSize;<br>
> - return reinterpret_cast<AsanChunk *>(chunk_beg);<br>
> + return 0;<br>
> }<br>
><br>
> static uptr AllocationSize(uptr p) {<br>
> @@ -489,14 +533,19 @@<br>
> // We have an address between two chunks, and we want to report just<br>
> one.<br>
> AsanChunk *ChooseChunk(uptr addr,<br>
> AsanChunk *left_chunk, AsanChunk<br>
> *right_chunk) {<br>
> - // Prefer an allocated chunk or a chunk from quarantine.<br>
> - if (left_chunk->chunk_state == CHUNK_AVAILABLE &&<br>
> - right_chunk->chunk_state != CHUNK_AVAILABLE)<br>
> - return right_chunk;<br>
> - if (right_chunk->chunk_state == CHUNK_AVAILABLE &&<br>
> - left_chunk->chunk_state != CHUNK_AVAILABLE)<br>
> - return left_chunk;<br>
> - // Choose based on offset.<br>
> + // Prefer an allocated chunk over freed chunk and freed chunk<br>
> + // over available chunk.<br>
> + if (left_chunk->chunk_state != right_chunk->chunk_state) {<br>
> + if (left_chunk->chunk_state == CHUNK_ALLOCATED)<br>
> + return left_chunk;<br>
> + if (right_chunk->chunk_state == CHUNK_ALLOCATED)<br>
> + return right_chunk;<br>
> + if (left_chunk->chunk_state == CHUNK_QUARANTINE)<br>
> + return left_chunk;<br>
> + if (right_chunk->chunk_state == CHUNK_QUARANTINE)<br>
> + return right_chunk;<br>
> + }<br>
> + // Same chunk_state: choose based on offset.<br>
> uptr l_offset = 0, r_offset = 0;<br>
> CHECK(AsanChunkView(left_chunk).AddrIsAtRight(addr, 1,<br>
> &l_offset));<br>
> CHECK(AsanChunkView(right_chunk).AddrIsAtLeft(addr, 1,<br>
> &r_offset));<br>
><br>
> Modified: compiler-rt/trunk/lib/asan/asan_rtl.cc<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/asan/asan_rtl.cc?rev=171107&r1=171106&r2=171107&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/asan/asan_rtl.cc?rev=171107&r1=171106&r2=171107&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/asan/asan_rtl.cc (original)<br>
> +++ compiler-rt/trunk/lib/asan/asan_rtl.cc Wed Dec 26 04:41:24 2012<br>
> @@ -117,7 +117,7 @@<br>
> f->quarantine_size = (ASAN_LOW_MEMORY) ? 1UL << 26 : 1UL << 28;<br>
> f->symbolize = false;<br>
> f->verbosity = 0;<br>
> - f->redzone = (ASAN_LOW_MEMORY) ? 64 : 128;<br>
> + f->redzone = ASAN_ALLOCATOR_VERSION == 2 ? 16 : (ASAN_LOW_MEMORY)<br>
> ? 64 : 128;<br>
> f->debug = false;<br>
> f->report_globals = 1;<br>
> f->check_initialization_order = true;<br>
><br>
> Modified: compiler-rt/trunk/lib/asan/tests/asan_test.cc<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/asan/tests/asan_test.cc?rev=171107&r1=171106&r2=171107&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/asan/tests/asan_test.cc?rev=171107&r1=171106&r2=171107&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/asan/tests/asan_test.cc (original)<br>
> +++ compiler-rt/trunk/lib/asan/tests/asan_test.cc Wed Dec 26 04:41:24<br>
> 2012<br>
> @@ -479,7 +479,6 @@<br>
><br>
> #ifndef __APPLE__<br>
> void MemalignRun(size_t align, size_t size, int idx) {<br>
> - fprintf(stderr, "align %ld\n", align);<br>
> char *p = (char *)memalign(align, size);<br>
> Ident(p)[idx] = 0;<br>
> free(p);<br>
> @@ -899,8 +898,11 @@<br>
> LeftOOBWriteMessage(1));<br>
> EXPECT_DEATH(memset((char*)array - 5, 0, 6),<br>
> LeftOOBWriteMessage(5));<br>
> - EXPECT_DEATH(memset(array - 5, element, size + 5 * sizeof(T)),<br>
> - LeftOOBWriteMessage(5 * sizeof(T)));<br>
> + if (length >= 100) {<br>
> + // Large OOB, we find it only if the redzone is large enough.<br>
> + EXPECT_DEATH(memset(array - 5, element, size + 5 * sizeof(T)),<br>
> + LeftOOBWriteMessage(5 * sizeof(T)));<br>
> + }<br>
> // whole interval is to the left<br>
> EXPECT_DEATH(memset(array - 2, 0, sizeof(T)),<br>
> LeftOOBWriteMessage(2 * sizeof(T)));<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div><br></div>