<div dir="ltr">We have about 600 lines of allocator tests, which is far too little.<div><br></div><div style>Regarding this specific change, you can't test it without allocating 4gb of memory (well, you can if you just feed the hand-crafted pointer to GetBlockBegin - but that's a bit hacky), and the change itself is so trivial that I don't think it's worth adding a regression test.</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, May 15, 2013 at 11:17 PM, Alexander Potapenko <span dir="ltr"><<a href="mailto:glider@google.com" target="_blank">glider@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><p dir="ltr">Do we have tests for the allocator? Looks like it's time to add some.</p>
<p dir="ltr">Sent from phone</p><div class="HOEnZb"><div class="h5">
<div class="gmail_quote">On May 15, 2013 11:11 PM, "Sergey Matveev" <<a href="mailto:earthdok@google.com" target="_blank">earthdok@google.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Hi kcc, glider,<br>
<br>
The 32-bit offset overflowed when more than 4GB was allocated in a size<br>
class. Also removed the misleading comment.<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D797" target="_blank">http://llvm-reviews.chandlerc.com/D797</a><br>
<br>
Files:<br>
  lib/sanitizer_common/sanitizer_allocator.h<br>
<br>
Index: lib/sanitizer_common/sanitizer_allocator.h<br>
===================================================================<br>
--- lib/sanitizer_common/sanitizer_allocator.h<br>
+++ lib/sanitizer_common/sanitizer_allocator.h<br>
@@ -492,11 +492,7 @@<br>
   }<br>
<br>
   static uptr GetChunkIdx(uptr chunk, uptr size) {<br>
-    u32 offset = chunk % kRegionSize;<br>
-    // Here we divide by a non-constant. This is costly.<br>
-    // We require that kRegionSize is at least 2^32 so that offset is 32-bit.<br>
-    // We save 2x by using 32-bit div, but may need to use a 256-way switch.<br>
-    return offset / (u32)size;<br>
+    return (chunk % kRegionSize) / size;<br>
   }<br>
<br>
   NOINLINE Batch* PopulateFreeList(AllocatorStats *stat, AllocatorCache *c,<br>
</blockquote></div>
</div></div></blockquote></div><br></div>