[compiler-rt] r287078 - fixing binary search for cases when element is not in array

Mike Aizatsky via llvm-commits llvm-commits at lists.llvm.org
Thu Nov 17 13:02:57 PST 2016


> Everything worked, right?

I am not sure. How do you think it worked?

The previous version of a function was BROKEN when you give it a value that
is not in the array. sometimes it returned size+1, sometimes some other
value, sometimes it dereferenced array with negative index (!) and
sometimes it went into forever loop.

Are you sure that you want to keep such a function around? I will happily
roll back my CL if you do.


On Thu, Nov 17, 2016 at 12:28 PM Kostya Serebryany <kcc at google.com> wrote:

> On Thu, Nov 17, 2016 at 12:12 PM, Mike Aizatsky <aizatsky at chromium.org>
> wrote:
>
> The existing one is incorrect anyway. It literally fails when element you
> look for is not in the array. I doubt it is ever the intended behavior.
>
>
> Everything worked, right?
>
> And this change made the call site less readable:
> -  if (idx > map_.size())
> +  if (idx > map_.size() || map_[idx].id != id)
>
>
> On Thu, Nov 17, 2016 at 10:24 Kostya Serebryany <kcc at google.com> wrote:
>
> On Thu, Nov 17, 2016 at 10:23 AM, Kostya Serebryany <kcc at google.com>
> wrote:
>
> I don't agree with this change.
> Instead of changing the semantics of the existing function
> InternalBinarySearch
> I would prefer to introduce a new one (InternalLowerBound).
> Of course, you could introduce one via another.
>
> ... implement one via another.
>
>
> On Tue, Nov 15, 2016 at 8:03 PM, Mike Aizatsky via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> Author: aizatsky
> Date: Tue Nov 15 22:03:27 2016
> New Revision: 287078
>
> URL: http://llvm.org/viewvc/llvm-project?rev=287078&view=rev
> Log:
> fixing binary search for cases when element is not in array
>
> Subscribers: kubabrecka
>
> Differential Revision: https://reviews.llvm.org/D26707
>
> Modified:
>     compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h
>     compiler-rt/trunk/lib/sanitizer_common/sanitizer_stackdepot.cc
>     compiler-rt/trunk/lib/sanitizer_common/tests/sanitizer_common_test.cc
>
> Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h
> URL:
> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h?rev=287078&r1=287077&r2=287078&view=diff
>
> ==============================================================================
> --- compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h (original)
> +++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_common.h Tue Nov 15
> 22:03:27 2016
> @@ -635,20 +635,19 @@ void InternalSort(Container *v, uptr siz
>    }
>  }
>
> +// Works like std::lower_bound: finds the first element that is not less
> +// than the val.
>  template<class Container, class Value, class Compare>
>  uptr InternalBinarySearch(const Container &v, uptr first, uptr last,
>                            const Value &val, Compare comp) {
> -  uptr not_found = last + 1;
> -  while (last >= first) {
> +  while (last > first) {
>      uptr mid = (first + last) / 2;
>      if (comp(v[mid], val))
>        first = mid + 1;
> -    else if (comp(val, v[mid]))
> -      last = mid - 1;
>      else
> -      return mid;
> +      last = mid;
>    }
> -  return not_found;
> +  return first;
>  }
>
>  // Represents a binary loaded into virtual memory (e.g. this can be an
>
> Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_stackdepot.cc
> URL:
> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_stackdepot.cc?rev=287078&r1=287077&r2=287078&view=diff
>
> ==============================================================================
> --- compiler-rt/trunk/lib/sanitizer_common/sanitizer_stackdepot.cc
> (original)
> +++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_stackdepot.cc Tue Nov
> 15 22:03:27 2016
> @@ -155,7 +155,7 @@ StackTrace StackDepotReverseMap::Get(u32
>    IdDescPair pair = {id, nullptr};
>    uptr idx = InternalBinarySearch(map_, 0, map_.size(), pair,
>                                    IdDescPair::IdComparator);
> -  if (idx > map_.size())
> +  if (idx > map_.size() || map_[idx].id != id)
>      return StackTrace();
>    return map_[idx].desc->load();
>  }
>
> Modified:
> compiler-rt/trunk/lib/sanitizer_common/tests/sanitizer_common_test.cc
> URL:
> 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
>
> ==============================================================================
> --- compiler-rt/trunk/lib/sanitizer_common/tests/sanitizer_common_test.cc
> (original)
> +++ compiler-rt/trunk/lib/sanitizer_common/tests/sanitizer_common_test.cc
> Tue Nov 15 22:03:27 2016
> @@ -10,6 +10,8 @@
>  // This file is a part of ThreadSanitizer/AddressSanitizer runtime.
>  //
>
>  //===----------------------------------------------------------------------===//
> +#include <algorithm>
> +
>  #include "sanitizer_common/sanitizer_allocator_internal.h"
>  #include "sanitizer_common/sanitizer_common.h"
>  #include "sanitizer_common/sanitizer_flags.h"
> @@ -172,13 +174,52 @@ bool UptrLess(uptr a, uptr b) {
>
>  TEST(SanitizerCommon, InternalBinarySearch) {
>    static const uptr kSize = 5;
> -  uptr arr[kSize];
> -  for (uptr i = 0; i < kSize; i++) arr[i] = i * i;
> +  int arr[kSize];
> +  arr[0] = 1;
> +  arr[1] = 3;
> +  arr[2] = 5;
> +  arr[3] = 7;
> +  arr[4] = 11;
> +
> +  EXPECT_EQ(0u, InternalBinarySearch(arr, 0, kSize, 0, UptrLess));
> +  EXPECT_EQ(0u, InternalBinarySearch(arr, 0, kSize, 1, UptrLess));
> +  EXPECT_EQ(1u, InternalBinarySearch(arr, 0, kSize, 2, UptrLess));
> +  EXPECT_EQ(1u, InternalBinarySearch(arr, 0, kSize, 3, UptrLess));
> +  EXPECT_EQ(2u, InternalBinarySearch(arr, 0, kSize, 4, UptrLess));
> +  EXPECT_EQ(2u, InternalBinarySearch(arr, 0, kSize, 5, UptrLess));
> +  EXPECT_EQ(3u, InternalBinarySearch(arr, 0, kSize, 6, UptrLess));
> +  EXPECT_EQ(3u, InternalBinarySearch(arr, 0, kSize, 7, UptrLess));
> +  EXPECT_EQ(4u, InternalBinarySearch(arr, 0, kSize, 8, UptrLess));
> +  EXPECT_EQ(4u, InternalBinarySearch(arr, 0, kSize, 9, UptrLess));
> +  EXPECT_EQ(4u, InternalBinarySearch(arr, 0, kSize, 10, UptrLess));
> +  EXPECT_EQ(4u, InternalBinarySearch(arr, 0, kSize, 11, UptrLess));
> +  EXPECT_EQ(5u, InternalBinarySearch(arr, 0, kSize, 12, UptrLess));
> +}
> +
> +TEST(SanitizerCommon, InternalBinarySearchVsLowerBound) {
> +  std::vector<int> data;
> +  auto create_item = [] (size_t i, size_t j) {
> +    auto v = i * 10000 + j;
> +    return ((v << 6) + (v >> 6) + 0x9e3779b9) % 100;
> +  };
> +  for (size_t i = 0; i < 1000; ++i) {
> +    data.resize(i);
> +    for (size_t j = 0; j < i; ++j) {
> +      data[j] = create_item(i, j);
> +    }
>
> -  for (uptr i = 0; i < kSize; i++)
> -    ASSERT_EQ(InternalBinarySearch(arr, 0, kSize, i * i, UptrLess), i);
> +    std::sort(data.begin(), data.end());
>
> -  ASSERT_EQ(InternalBinarySearch(arr, 0, kSize, 7, UptrLess), kSize + 1);
> +    for (size_t j = 0; j < i; ++j) {
> +      int val = create_item(i, j);
> +      for (auto to_find : {val - 1, val, val + 1}) {
> +        uptr expected =
> +            std::lower_bound(data.begin(), data.end(), to_find) -
> data.begin();
> +        EXPECT_EQ(expected, InternalBinarySearch(data.data(), 0,
> data.size(),
> +                                                 to_find,
> std::less<int>()));
> +      }
> +    }
> +  }
>  }
>
>  #if SANITIZER_LINUX && !SANITIZER_ANDROID
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161117/405d771f/attachment.html>


More information about the llvm-commits mailing list