commit fc3463da017a9e17e890363977ec29f90c82411b
author Jamie Madill <jmadill@chromium.org> Wed Jan 03 13:46:21 2018 -0500
committer Commit Bot <commit-bot@chromium.org> Wed Jan 03 21:26:55 2018 +0000
tree 1996dcb67e2a4173598e0cc4d6558822f820752e
parent 36937a64f438a2e0c166f99d90ced59c5dbbb1be

HandleAllocator: Fix heap ordering using std::greater.

The default heap ordering is to return the greatest element in the
heap. Since the handle allocator expects a minimal return value on
a new allocation, this caused a bug. The bug is triggered by reserving
handles, allocating new handles, then freeing the handles and
allocating again with the same allocator. Fix the bug by using
std::greater instead of std::less, which will make the heap
return the smallest value instead of largest.

Also adds some logging debugging code for the handle allocators.

Bug: angleproject:1458
Change-Id: Ibef5dcbed0a664ccad0e0335f081e2355162584b
Reviewed-on: https://chromium-review.googlesource.com/848644
Commit-Queue: Jamie Madill <jmadill@chromium.org>
Reviewed-by: Frank Henigman <fjhenigman@chromium.org>

src/libANGLE/HandleAllocator.cpp

diff --git a/src/libANGLE/HandleAllocator.cpp b/src/libANGLE/HandleAllocator.cpp
index c3c1842..c0c0fa2 100644
--- a/src/libANGLE/HandleAllocator.cpp
+++ b/src/libANGLE/HandleAllocator.cpp
@@ -10,6 +10,7 @@
 #include "libANGLE/HandleAllocator.h"
 
 #include <algorithm>
+#include <functional>
 
 #include "common/debug.h"
 
@@ -24,7 +25,7 @@
     }
 };
 
-HandleAllocator::HandleAllocator() : mBaseValue(1), mNextValue(1)
+HandleAllocator::HandleAllocator() : mBaseValue(1), mNextValue(1), mLoggingEnabled(false)
 {
     mUnallocatedList.push_back(HandleRange(1, std::numeric_limits<GLuint>::max()));
 }
@@ -52,9 +53,15 @@
     // Allocate from released list, logarithmic time for pop_heap.
     if (!mReleasedList.empty())
     {
-        std::pop_heap(mReleasedList.begin(), mReleasedList.end());
+        std::pop_heap(mReleasedList.begin(), mReleasedList.end(), std::greater<GLuint>());
         GLuint reusedHandle = mReleasedList.back();
         mReleasedList.pop_back();
+
+        if (mLoggingEnabled)
+        {
+            WARN() << "HandleAllocator::allocate reusing " << reusedHandle << std::endl;
+        }
+
         return reusedHandle;
     }
 
@@ -73,18 +80,33 @@
         listIt->begin++;
     }
 
+    if (mLoggingEnabled)
+    {
+        WARN() << "HandleAllocator::allocate allocating " << freeListHandle << std::endl;
+    }
+
     return freeListHandle;
 }
 
 void HandleAllocator::release(GLuint handle)
 {
+    if (mLoggingEnabled)
+    {
+        WARN() << "HandleAllocator::release releasing " << handle << std::endl;
+    }
+
     // Add to released list, logarithmic time for push_heap.
     mReleasedList.push_back(handle);
-    std::push_heap(mReleasedList.begin(), mReleasedList.end());
+    std::push_heap(mReleasedList.begin(), mReleasedList.end(), std::greater<GLuint>());
 }
 
 void HandleAllocator::reserve(GLuint handle)
 {
+    if (mLoggingEnabled)
+    {
+        WARN() << "HandleAllocator::reserve reserving " << handle << std::endl;
+    }
+
     // Clear from released list -- might be a slow operation.
     if (!mReleasedList.empty())
     {
@@ -139,4 +161,9 @@
     mNextValue = 1;
 }
 
+void HandleAllocator::enableLogging(bool enabled)
+{
+    mLoggingEnabled = enabled;
+}
+
 }  // namespace gl

src/libANGLE/HandleAllocator.h

diff --git a/src/libANGLE/HandleAllocator.h b/src/libANGLE/HandleAllocator.h
index cd21d68..5af5627 100644
--- a/src/libANGLE/HandleAllocator.h
+++ b/src/libANGLE/HandleAllocator.h
@@ -34,6 +34,8 @@
     void reserve(GLuint handle);
     void reset();
 
+    void enableLogging(bool enabled);
+
   private:
     GLuint mBaseValue;
     GLuint mNextValue;
@@ -56,6 +58,8 @@
     // released, stored in a heap.
     std::vector<HandleRange> mUnallocatedList;
     std::vector<GLuint> mReleasedList;
+
+    bool mLoggingEnabled;
 };
 
 }  // namespace gl

src/libANGLE/HandleAllocator_unittest.cpp

diff --git a/src/libANGLE/HandleAllocator_unittest.cpp b/src/libANGLE/HandleAllocator_unittest.cpp
index d975b4b..8597c15 100644
--- a/src/libANGLE/HandleAllocator_unittest.cpp
+++ b/src/libANGLE/HandleAllocator_unittest.cpp
@@ -150,4 +150,24 @@
     }
 }
 
+// Covers a particular bug with reserving and allocating sub ranges.
+TEST(HandleAllocatorTest, ReserveAndAllocateIterated)
+{
+    gl::HandleAllocator allocator;
+
+    for (int iteration = 0; iteration < 3; ++iteration)
+    {
+        allocator.reserve(5);
+        allocator.reserve(6);
+        GLuint a = allocator.allocate();
+        GLuint b = allocator.allocate();
+        GLuint c = allocator.allocate();
+        allocator.release(c);
+        allocator.release(a);
+        allocator.release(b);
+        allocator.release(5);
+        allocator.release(6);
+    }
+}
+
 }  // anonymous namespace

src/libANGLE/ResourceManager.cpp

diff --git a/src/libANGLE/ResourceManager.cpp b/src/libANGLE/ResourceManager.cpp
index 79eb7e5..ebff5e5 100644
--- a/src/libANGLE/ResourceManager.cpp
+++ b/src/libANGLE/ResourceManager.cpp
@@ -260,6 +260,11 @@
     }
 }
 
+void TextureManager::enableHandleAllocatorLogging()
+{
+    mHandleAllocator.enableLogging(true);
+}
+
 // RenderbufferManager Implementation.
 
 // static

src/libANGLE/ResourceManager.h

diff --git a/src/libANGLE/ResourceManager.h b/src/libANGLE/ResourceManager.h
index 2dfeff5..4dadca6 100644
--- a/src/libANGLE/ResourceManager.h
+++ b/src/libANGLE/ResourceManager.h
@@ -167,6 +167,8 @@
     static Texture *AllocateNewObject(rx::GLImplFactory *factory, GLuint handle, GLenum target);
     static void DeleteObject(const Context *context, Texture *texture);
 
+    void enableHandleAllocatorLogging();
+
   protected:
     ~TextureManager() override {}
 };