<div dir="ltr">I don't agree with this change. <div>Instead of changing the semantics of the existing function<span style="font-size:12.8px"> </span><span style="font-size:12.8px">InternalBinarySearch</span></div><div><span style="font-size:12.8px">I would prefer to introduce a new one (</span><span style="font-size:12.8px">InternalLowerBound). </span></div><div><span style="font-size:12.8px">Of course, you could introduce one via another. </span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 15, 2016 at 8:03 PM, Mike Aizatsky via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: aizatsky<br>
Date: Tue Nov 15 22:03:27 2016<br>
New Revision: 287078<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=287078&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=287078&view=rev</a><br>
Log:<br>
fixing binary search for cases when element is not in array<br>
<br>
Subscribers: kubabrecka<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D26707" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D26707</a><br>
<br>
Modified:<br>
compiler-rt/trunk/lib/<wbr>sanitizer_common/sanitizer_<wbr>common.h<br>
compiler-rt/trunk/lib/<wbr>sanitizer_common/sanitizer_<wbr>stackdepot.cc<br>
compiler-rt/trunk/lib/<wbr>sanitizer_common/tests/<wbr>sanitizer_common_test.cc<br>
<br>
Modified: compiler-rt/trunk/lib/<wbr>sanitizer_common/sanitizer_<wbr>common.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h?rev=287078&r1=287077&r2=287078&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/compiler-rt/trunk/lib/<wbr>sanitizer_common/sanitizer_<wbr>common.h?rev=287078&r1=287077&<wbr>r2=287078&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- compiler-rt/trunk/lib/<wbr>sanitizer_common/sanitizer_<wbr>common.h (original)<br>
+++ compiler-rt/trunk/lib/<wbr>sanitizer_common/sanitizer_<wbr>common.h Tue Nov 15 22:03:27 2016<br>
@@ -635,20 +635,19 @@ void InternalSort(Container *v, uptr siz<br>
}<br>
}<br>
<br>
+// Works like std::lower_bound: finds the first element that is not less<br>
+// than the val.<br>
template<class Container, class Value, class Compare><br>
uptr InternalBinarySearch(const Container &v, uptr first, uptr last,<br>
const Value &val, Compare comp) {<br>
- uptr not_found = last + 1;<br>
- while (last >= first) {<br>
+ while (last > first) {<br>
uptr mid = (first + last) / 2;<br>
if (comp(v[mid], val))<br>
first = mid + 1;<br>
- else if (comp(val, v[mid]))<br>
- last = mid - 1;<br>
else<br>
- return mid;<br>
+ last = mid;<br>
}<br>
- return not_found;<br>
+ return first;<br>
}<br>
<br>
// Represents a binary loaded into virtual memory (e.g. this can be an<br>
<br>
Modified: compiler-rt/trunk/lib/<wbr>sanitizer_common/sanitizer_<wbr>stackdepot.cc<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_stackdepot.cc?rev=287078&r1=287077&r2=287078&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/compiler-rt/trunk/lib/<wbr>sanitizer_common/sanitizer_<wbr>stackdepot.cc?rev=287078&r1=<wbr>287077&r2=287078&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- compiler-rt/trunk/lib/<wbr>sanitizer_common/sanitizer_<wbr>stackdepot.cc (original)<br>
+++ compiler-rt/trunk/lib/<wbr>sanitizer_common/sanitizer_<wbr>stackdepot.cc Tue Nov 15 22:03:27 2016<br>
@@ -155,7 +155,7 @@ StackTrace StackDepotReverseMap::Get(u32<br>
IdDescPair pair = {id, nullptr};<br>
uptr idx = InternalBinarySearch(map_, 0, map_.size(), pair,<br>
IdDescPair::IdComparator);<br>
- if (idx > map_.size())<br>
+ if (idx > map_.size() || map_[idx].id != id)<br>
return StackTrace();<br>
return map_[idx].desc->load();<br>
}<br>
<br>
Modified: compiler-rt/trunk/lib/<wbr>sanitizer_common/tests/<wbr>sanitizer_common_test.cc<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/tests/sanitizer_common_test.cc?rev=287078&r1=287077&r2=287078&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/compiler-rt/trunk/lib/<wbr>sanitizer_common/tests/<wbr>sanitizer_common_test.cc?rev=<wbr>287078&r1=287077&r2=287078&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- compiler-rt/trunk/lib/<wbr>sanitizer_common/tests/<wbr>sanitizer_common_test.cc (original)<br>
+++ compiler-rt/trunk/lib/<wbr>sanitizer_common/tests/<wbr>sanitizer_common_test.cc Tue Nov 15 22:03:27 2016<br>
@@ -10,6 +10,8 @@<br>
// This file is a part of ThreadSanitizer/<wbr>AddressSanitizer runtime.<br>
//<br>
//===-------------------------<wbr>------------------------------<wbr>---------------===//<br>
+#include <algorithm><br>
+<br>
#include "sanitizer_common/sanitizer_<wbr>allocator_internal.h"<br>
#include "sanitizer_common/sanitizer_<wbr>common.h"<br>
#include "sanitizer_common/sanitizer_<wbr>flags.h"<br>
@@ -172,13 +174,52 @@ bool UptrLess(uptr a, uptr b) {<br>
<br>
TEST(SanitizerCommon, InternalBinarySearch) {<br>
static const uptr kSize = 5;<br>
- uptr arr[kSize];<br>
- for (uptr i = 0; i < kSize; i++) arr[i] = i * i;<br>
+ int arr[kSize];<br>
+ arr[0] = 1;<br>
+ arr[1] = 3;<br>
+ arr[2] = 5;<br>
+ arr[3] = 7;<br>
+ arr[4] = 11;<br>
+<br>
+ EXPECT_EQ(0u, InternalBinarySearch(arr, 0, kSize, 0, UptrLess));<br>
+ EXPECT_EQ(0u, InternalBinarySearch(arr, 0, kSize, 1, UptrLess));<br>
+ EXPECT_EQ(1u, InternalBinarySearch(arr, 0, kSize, 2, UptrLess));<br>
+ EXPECT_EQ(1u, InternalBinarySearch(arr, 0, kSize, 3, UptrLess));<br>
+ EXPECT_EQ(2u, InternalBinarySearch(arr, 0, kSize, 4, UptrLess));<br>
+ EXPECT_EQ(2u, InternalBinarySearch(arr, 0, kSize, 5, UptrLess));<br>
+ EXPECT_EQ(3u, InternalBinarySearch(arr, 0, kSize, 6, UptrLess));<br>
+ EXPECT_EQ(3u, InternalBinarySearch(arr, 0, kSize, 7, UptrLess));<br>
+ EXPECT_EQ(4u, InternalBinarySearch(arr, 0, kSize, 8, UptrLess));<br>
+ EXPECT_EQ(4u, InternalBinarySearch(arr, 0, kSize, 9, UptrLess));<br>
+ EXPECT_EQ(4u, InternalBinarySearch(arr, 0, kSize, 10, UptrLess));<br>
+ EXPECT_EQ(4u, InternalBinarySearch(arr, 0, kSize, 11, UptrLess));<br>
+ EXPECT_EQ(5u, InternalBinarySearch(arr, 0, kSize, 12, UptrLess));<br>
+}<br>
+<br>
+TEST(SanitizerCommon, InternalBinarySearchVsLowerBou<wbr>nd) {<br>
+ std::vector<int> data;<br>
+ auto create_item = [] (size_t i, size_t j) {<br>
+ auto v = i * 10000 + j;<br>
+ return ((v << 6) + (v >> 6) + 0x9e3779b9) % 100;<br>
+ };<br>
+ for (size_t i = 0; i < 1000; ++i) {<br>
+ data.resize(i);<br>
+ for (size_t j = 0; j < i; ++j) {<br>
+ data[j] = create_item(i, j);<br>
+ }<br>
<br>
- for (uptr i = 0; i < kSize; i++)<br>
- ASSERT_EQ(<wbr>InternalBinarySearch(arr, 0, kSize, i * i, UptrLess), i);<br>
+ std::sort(data.begin(), data.end());<br>
<br>
- ASSERT_EQ(<wbr>InternalBinarySearch(arr, 0, kSize, 7, UptrLess), kSize + 1);<br>
+ for (size_t j = 0; j < i; ++j) {<br>
+ int val = create_item(i, j);<br>
+ for (auto to_find : {val - 1, val, val + 1}) {<br>
+ uptr expected =<br>
+ std::lower_bound(data.begin(), data.end(), to_find) - data.begin();<br>
+ EXPECT_EQ(expected, InternalBinarySearch(data.<wbr>data(), 0, data.size(),<br>
+ to_find, std::less<int>()));<br>
+ }<br>
+ }<br>
+ }<br>
}<br>
<br>
#if SANITIZER_LINUX && !SANITIZER_ANDROID<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>