<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Zach,<div><br></div><div>sorry for the very long wait for this review.  My mail filters haven’t been set up to promote patch e-mails from the lldb-commits ML to my main inbox; I’ll make sure to track them properly going forward.</div><div><br></div><div>We want to create allocations that don’t conflict with underlying process allocations because of this scenario:</div><div><br></div><div><ul class="MailOutline"><li>You’re running on some kind of device that can’t allocate memory (e.g., a dev board or a kernel core)</li><li>You run an expression that sets up some kind of data structure and uses pointers to address into it</li><li>That expression also needs to use data from the underlying image</li></ul><div><br></div></div><div>Now if you haven’t made sure that you aren’t conflicting with underlying allocations, you can get the wrong data when you use data from the underlying image.  That’s what this code is meant to avoid.</div><div><br></div><div>If there’s a better way to do it, then great!  I used to have pointers in the IRInterpreter be annotated with their origin but it was error-prone to maintain that illusion throughout the whole run of the expression, which might store the pointer somewhere (including but not limited to somewhere in the underlying process!) and then re-use it.  </div><div><br></div><div>That said, there are a few problems with the existing algorithm besides what you mentioned.</div><div><br></div><div><ul class="MailOutline"><li>You could allocate [0x100, 0x200) happily, checking each end, but there’s an allocation in the underlying process at [0x150, 0x160).  If you make certain assumptions about allocation size (e.g., that allocations are in multiples of the page size) you can eliminate this by probing at intervals between the start and end.</li><li>The probing costs round-trip times to the underlying process.  Worse, on a dev board it can probe locations that should not be read (e.g., mapped registers).</li></ul><div><br></div></div><div>For these reasons I’d actually lean toward using something like your patch.  But I don’t like the policy of “Don’t use the zero page” – that seems like a fairly ad-hoc, target-specific limitation.  I’d prefer to use the image list to avoid areas we *know* are mapped.</div><div><br></div><div><div>
<div>Sean</div>

</div>
<br><div><blockquote type="cite"><div>On Jun 25, 2014, at 4:15 PM, Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div>The current strategy for host allocation is to choose a random address and attempt to allocate there, eventually failing if the allocation cannot be satisfied.<br><br>The C standard only guarantees that RAND_MAX >= 32767, so for platforms where this is true allocations will fail with very high probability.   On such platforms, you can reproduce this trivially by running lldb, typing "expr (3)" and then hitting enter you see a failure.  Failures generally happen with a frequency of about 1 failure every 5 evaluations.<br><br>I cannot come up with a good reason that the allocations need to look like "real" pointers, so this patch changes the allocation scheme to simply jump straight to the end and grab a free chunk of memory.<br><br><a href="http://reviews.llvm.org/D4300">http://reviews.llvm.org/D4300</a><br><br>Files:<br>  source/Expression/IRMemoryMap.cpp<br><br>Index: source/Expression/IRMemoryMap.cpp<br>===================================================================<br>--- source/Expression/IRMemoryMap.cpp<br>+++ source/Expression/IRMemoryMap.cpp<br>@@ -53,6 +53,8 @@<br>     lldb::ProcessSP process_sp = m_process_wp.lock();<br><br>     lldb::addr_t ret = LLDB_INVALID_ADDRESS;<br>+    if (size == 0)<br>+      return ret;<br><br>     if (process_sp && process_sp->CanJIT() && process_sp->IsAlive())<br>     {<br>@@ -66,37 +68,13 @@<br>             return ret;<br>     }<br><br>-    for (int iterations = 0; iterations < 16; ++iterations)<br>-    {<br>-        lldb::addr_t candidate = LLDB_INVALID_ADDRESS;<br>-        <br>-        switch (target_sp->GetArchitecture().GetAddressByteSize())<br>-        {<br>-        case 4:<br>-            {<br>-                uint32_t random_data = rand();<br>-                candidate = random_data;<br>-                candidate &= ~0xfffull;<br>-                break;<br>-            }<br>-        case 8:<br>-            {<br>-                uint32_t random_low = rand();<br>-                uint32_t random_high = rand();<br>-                candidate = random_high;<br>-                candidate <<= 32ull;<br>-                candidate |= random_low;<br>-                candidate &= ~0xfffull;<br>-                break;<br>-            }<br>-        }<br>-        <br>-        if (IntersectsAllocation(candidate, size))<br>-            continue;<br>-                <br>-        ret = candidate;<br>-            <br>-        return ret;<br>+    // Don't allocate from the zero page.<br>+    ret = 0x1000;<br>+    if (!m_allocations.empty()) {<br>+      auto back = m_allocations.rbegin();<br>+      lldb::addr_t addr = back->first;<br>+      size_t size = back->second.m_size;<br>+      ret = llvm::RoundUpToAlignment(addr+size, 4096);<br>     }<br><br>     return ret;<br><span><D4300.10860.patch></span>_______________________________________________<br>lldb-commits mailing list<br>lldb-commits@cs.uiuc.edu<br>http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits<br></div></blockquote></div><br></div></body></html>