<div dir="ltr">I should have commit rights. I will make the changes you requested and upload.<div><br></div><div>Thanks for the review!</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jun 25, 2014 at 11:07 AM, Sean Callanan <span dir="ltr"><<a href="mailto:scallanan@apple.com" target="_blank">scallanan@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The IntersectsAllocation logic looks fine to me.<br>
<br>
I would change the second, four-argument version that you added be (a) static and (b) named differently (e.g., “AllocationsIntersect”) to make it clear that this is not performing the same service as the first function.<br>
<br>
Making the two-argument version of IntersectsAllocation const is good. Thanks for the cleanup.<br>
<br>
The algorithmic cleanup looks fine too.<br>
<br>
Please provide an updated patch and I’ll commit if you don’t have that right.<br>
<br>
Sean<br>
<div><div class="h5"><br>
> On Jun 25, 2014, at 1:09 AM, Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br>
><br>
> The IRMemoryMap represents a set of disjoint allocations, stored as an interval map. A bug in its allocation algorithm would cause this disjoint property to be violated, resulting in overlapping intervals in the map.<br>
><br>
> Consider the following scenario:<br>
><br>
> Allocation 1: 8 bytes, address 8192 is chosen by the allocator<br>
> Allocation 2: 524288, address 0 is probed<br>
><br>
> Without this patch, the IntersectsAllocation() method would behave as follows:<br>
><br>
> 1) lower_bound(0) returns an iterator referring to allocation 1<br>
> 2) 0 is compared against 8192, and since 0 < 8192, it says there is no intersection.<br>
><br>
> With the patch, it does proper interval tests.<br>
><br>
> <a href="http://reviews.llvm.org/D4286" target="_blank">http://reviews.llvm.org/D4286</a><br>
><br>
> Files:<br>
> include/lldb/Expression/IRMemoryMap.h<br>
> source/Expression/IRMemoryMap.cpp<br>
><br>
> Index: include/lldb/Expression/IRMemoryMap.h<br>
> ===================================================================<br>
> --- include/lldb/Expression/IRMemoryMap.h<br>
> +++ include/lldb/Expression/IRMemoryMap.h<br>
> @@ -27,7 +27,8 @@<br>
> /// This class encapsulates a group of memory objects that must be readable<br>
> /// or writable from the host process regardless of whether the process<br>
> /// exists. This allows the IR interpreter as well as JITted code to access<br>
> -/// the same memory.<br>
> +/// the same memory. All allocations made by this class are represented as<br>
> +/// disjoint intervals.<br>
> ///<br>
> /// Point queries against this group of memory objects can be made by the<br>
> /// address in the tar at which they reside. If the inferior does not<br>
> @@ -118,7 +119,12 @@<br>
> lldb::addr_t FindSpace (size_t size);<br>
> bool ContainsHostOnlyAllocations ();<br>
> AllocationMap::iterator FindAllocation (lldb::addr_t addr, size_t size);<br>
> - bool IntersectsAllocation (lldb::addr_t addr, size_t size);<br>
> +<br>
> + // Returns true if the given allocation intersects any allocation in the memory map.<br>
> + bool IntersectsAllocation (lldb::addr_t addr, size_t size) const;<br>
> +<br>
> + // Returns true if the two given allocations intersect each other.<br>
> + bool IntersectsAllocation (lldb::addr_t addr1, size_t size1, lldb::addr_t addr2, size_t size2) const;<br>
> };<br>
><br>
> }<br>
> Index: source/Expression/IRMemoryMap.cpp<br>
> ===================================================================<br>
> --- source/Expression/IRMemoryMap.cpp<br>
> +++ source/Expression/IRMemoryMap.cpp<br>
> @@ -125,33 +125,48 @@<br>
> }<br>
><br>
> bool<br>
> -IRMemoryMap::IntersectsAllocation (lldb::addr_t addr, size_t size)<br>
> +IRMemoryMap::IntersectsAllocation (lldb::addr_t addr, size_t size) const<br>
> {<br>
> if (addr == LLDB_INVALID_ADDRESS)<br>
> return false;<br>
><br>
> - AllocationMap::iterator iter = m_allocations.lower_bound (addr);<br>
> + AllocationMap::const_iterator iter = m_allocations.lower_bound (addr);<br>
><br>
> - if (iter == m_allocations.end() ||<br>
> - iter->first > addr)<br>
> - {<br>
> - if (iter == m_allocations.begin())<br>
> - return false;<br>
> -<br>
> - iter--;<br>
> + // Since we only know that the returned interval begins at a location greater than or<br>
> + // equal to where the given interval begins, it's possible that the given interval<br>
> + // intersects either the returned interval or the previous interval. Thus, we need to<br>
> + // check both. Note that we only need to check these two intervals. Since all intervals<br>
> + // are disjoint it is not possible that an adjacent interval does not intersect, but a<br>
> + // non-adjacent interval does intersect.<br>
> + if (iter != m_allocations.end()) {<br>
> + if (IntersectsAllocation(addr, size, iter->second.m_process_start, iter->second.m_size))<br>
> + return true;<br>
> }<br>
> -<br>
> - while (iter != m_allocations.end() && iter->second.m_process_alloc < addr + size)<br>
> - {<br>
> - if (iter->second.m_process_start + iter->second.m_size > addr)<br>
> +<br>
> + if (iter != m_allocations.begin()) {<br>
> + --iter;<br>
> + if (IntersectsAllocation(addr, size, iter->second.m_process_start, iter->second.m_size))<br>
> return true;<br>
> -<br>
> - ++iter;<br>
> }<br>
> -<br>
> +<br>
> return false;<br>
> }<br>
><br>
> +bool<br>
> +IRMemoryMap::IntersectsAllocation(lldb::addr_t addr1, size_t size1, lldb::addr_t addr2, size_t size2) const {<br>
> + // Given two half open intervals [A, B) and [X, Y), the only 6 permutations that satisfy<br>
> + // A<B and X<Y are the following:<br>
> + // A B X Y<br>
> + // A X B Y (intersects)<br>
> + // A X Y B (intersects)<br>
> + // X A B Y (intersects)<br>
> + // X A Y B (intersects)<br>
> + // X Y A B<br>
> + // The first is B <= X, and the last is Y <= A.<br>
> + // So the condition is !(B <= X || Y <= A)), or (X < B && A < Y)<br>
> + return (addr2 < (addr1 + size1)) && (addr1 < (addr2 + size2));<br>
> +}<br>
> +<br>
> lldb::ByteOrder<br>
> IRMemoryMap::GetByteOrder()<br>
> {<br>
</div></div>> <D4286.10822.patch>_______________________________________________<br>
> lldb-commits mailing list<br>
> <a href="mailto:lldb-commits@cs.uiuc.edu">lldb-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
<br>
</blockquote></div><br></div>